sec. vuln.: brackets {{ and }} in URL can trigger template engine

[capc0]

If any request constains {{ or }} in the URL, e.g.

http://url.tld?param={{dummy}}

the following log always errors.

        app.use(expressWinstonLogger({
            winstonInstance: logger,
            msg: (req: APIRequest, res) => {
                return `HTTP ${req.url}`;
            },
            colorize: true,
        }));

with

ReferenceError: dummy is not defined
    at eval (lodash.templateSources[4]:9:10)
    at C:\Users\Simon\Documents\Development\api\node_modules\express-winston\index.js:160:46
    at ServerResponse.res.end (C:\Users\Simon\Documents\Development\api\node_modules\express-winston\index.js:419:23)
    at ServerResponse.send (C:\Users\Simon\Documents\Development\api\node_modules\express\lib\response.js:221:10)

this might also cause some security issues.

http://url.tld?param={{console.log(1)}} actually prints 1 in the console...

How can I disable the template engine, since I provide my own msg function?

[capc0]

Is this lib still maintained? This is a not-so-small security issue...

requests like url?{{process.exit(1)}} will kill APIs

[bithavoc]

Sorry I missed this @capc0, that looks pretty bad indeed. We have a plan to move our lodash dependency to a separate library in the 5.x series, is currently the source of most of the security issues in the library, what do you think we can do for now?

[capc0]

I suggest removing the usage of the template engine when a custom message function is declared.

So alwas return m in https://github.com/bithavoc/express-winston/blob/main/index.js#L154 and so avoid running https://github.com/bithavoc/express-winston/blob/main/index.js#L160

Maybe create a property within loggerOptions to disable the feature.

[jnsppp]

Hey @bithavoc is this package still maintained?

[bithavoc]

Hey @bithavoc is this package still maintained?

short answer: yes
long answer: we had some appointed maintainers but seems most have faded away, I can take the helm back.

I can't fix this right now and the typescript + lodash split seems like a long effort.

I can accept a PR to fix this specific issue though.

[jnsppp]

Hey @bithavoc is this package still maintained?

short answer: yes long answer: we had some appointed maintainers but seems most have faded away, I can take the helm back.

I can't fix this right now and the typescript + lodash split seems like a long effort.

I can accept a PR to fix this specific issue though.

Okay, thanks for your reply @bithavoc!

I have a quick fix in mind, that would basically follow the suggestion @capc0 made.

Can you give me write permissions to this repo so that I can open a PR? Thanks!

[bithavoc]

Hey @bithavoc is this package still maintained?

short answer: yes long answer: we had some appointed maintainers but seems most have faded away, I can take the helm back.
I can't fix this right now and the typescript + lodash split seems like a long effort.
I can accept a PR to fix this specific issue though.

Okay, thanks for your reply @bithavoc!

I have a quick fix in mind, that would basically follow the suggestion @capc0 made.

Can you give me write permissions to this repo so that I can open a PR? Thanks!

you don't need write permission to the repo, you're welcome to fork and send a Pull Request

[jnsppp]

Hey @bithavoc is this package still maintained?

short answer: yes long answer: we had some appointed maintainers but seems most have faded away, I can take the helm back.
I can't fix this right now and the typescript + lodash split seems like a long effort.
I can accept a PR to fix this specific issue though.

Okay, thanks for your reply @bithavoc!
I have a quick fix in mind, that would basically follow the suggestion @capc0 made.
Can you give me write permissions to this repo so that I can open a PR? Thanks!

you don't need write permission to the repo, you're welcome to fork and send a Pull Request

Sure, here's the PR:#284

[thislooksfun]

This is a critical security vulnerability. Like CVE-level critical. Arbitrary remote code execution means an attacker could do anything on your server, potentially without you knowing. I won't share code snippets for security reasons, but I have confirmed that a vulnerable server could be exploited to read/write arbitrary files, read/write to any databases it has access too, and exfiltrate the full environment variables of the host. I'm sure there's more a sophisticated hacker could do as well. This needs to be fixed and a security advisory published ASAP.

[lynoure]

@bithavoc The comment from @thislooksfun is on point. Please see if you can accept that pull request. It can hardly make the situation worse unless it's actively malicious.