Semgrep: JS browser security concat bypass not detected

[throwaway-semgrep]

Describe the bug
Context: This is a throw away account to preserve my privacy. I am a dev at a Fortune 500 and my manager is an a@@ h@@@ who doesn't understand or cares about security. Recently one of our clients needed a security audit for the coding practices and we used tools like semgrep, SonarQube etc. My manager had forced us to use jquery with string concat to create html elements instead of dynamic html components creation. We all protested this but he didn't agree citing timelines. So during the security audit Semgrep found "raw-html-concat" issues and I was happy that atleast now he will be forced to follow proper standards. But instead he bypassed me and forced the juniors to use concat() instead of string concat. And this was undetected by semgrep.

For example, in your example if he was incharge of fixing he would have converted

return '<div align="' + (attrs.defaultattr || 'left') + '">' + content + '</div>';

to

return '<div align="'.concat(attrs.defaultattr || 'left').concat('">'.concat(content)).concat('</div>');

The changes my juniors has done to the code base is pathetic but it passes all tests and goes unnoticed. Common sense dictates that nobody in their right mind would do something like this to beautiful code, but I'm letting you know that these kind of people does exist. Thank you for your efforts. Please add additional patterns to capture this.

To Reproduce
Convert string concat with + to concat()

Expected behavior
Even if user tries to trick semgrep it should be noticed

[underyx]

This story was so sad, I had to immediately jump on the issue, and I submitted a PR improving on the rule: https://github.com/returntocorp/semgrep-rules/pull/1070

Of course this is just a quick band-aid that I added mainly for the benefit of other innocent devs who are using .concat() in good faith. Unfortunately I feel like we would inevitably lose any game of whack-a-mole where users are trying to find ways to circumvent Semgrep's security advice. I can think of five other ways the rule could be circumvented, and I have no doubt your manager can, too. Ultimately, Semgrep (and pretty much any other security tooling) can only work with the users' cooperation; a team that's not blatantly ignoring security would be happy whenever Semgrep reports a true positive result — they can be that much more confident they won't have a breach.

To solve your underlying issue, I don't think I can do much, but I would recommend considering what political and economic factors led to this approach within your organization. Ask questions like: Why is there no fear of a security breach? Is it because the risk is underestimated? Is it because the cost is legitimately low due to some weird reason? Is it because the cost threatens someone other than the decision maker, e.g. if they can deflect blame? Why do contracts allow for shenanigans like this? Why is there toxic pressure to deliver fast? Why does this manager ignore how morale drops when engineers are forced to write poor code? Do other members on the team not care? Or does the manager not realize this is a cause of bad morale? Do they realize but have no incentive to care? And ultimately, are these demotivating circumstances and disincentives to honing your engineering craftsmanship worth the salary you get there?

I'm sorry if this might feel a bit too confrontational, that's not my intention. I also understand that there's countless circumstances that would force one to stay at a job like this, so there's no reason to feel like that's the wrong choice. I just felt deeply sad about your situation and wanted to offer some advice to improve on it or at the very least, help make sense of it. Good luck!

[download13]

I'm guessing that manager is not going to allow anyone to update semgrep...

[SirJson]

How can a manager dictate how you design a program? I'm not saying you made this story up somehow, I'm just baffled that this is even a thing.

[EternityForest]

@SirJson I'm not really surprised honestly. Some managers are absolutely obsessed with simplicity. I very, very much doubt this will save even 5 seconds compared to doing in right.

But it has the right kind of "feel" of good old, common sense, direct control, not using what seems like advanced features(Aka stuff they don't understand right now), etc.

I think that whenever someone does stuff and you think "What were they thinking?", the answer is that they weren't. They were using rules of thumb and gut feelings, and somehow, whenever people do that, they always seem to go for whatever solution involves doing more of the low level stuff yourself. It's not rational, it's just a "we don't need that" instinct cranked up to 11.

It feels subjectively extremely productive to do this because you never context switch. You can just code constantly and never RTFM. Obviously, five minutes reading is less than an hour coding, but to them it feels like an interrupting.

These are non-technical ish people with business training, and a shelf of success books telling them to be confident and not overthink. By never bothering to learn to do it right, they can feel like they're just absolutely flying through the code.

And of course, micromanaging in general has always been a thing.

[jdmarshall]

@underyx Do you also detect join()? Because we all know that's the next stop on this roller coaster.

[sdegueldre]

If you're wondering why there is a sudden uptick in the activity of this issue, it's because it has been linked on reddit, probably because it's both funny and sad and that makes for good reddit content. Anyhow, as you said, it's an arm race you cannot win with static analysis. If someone wants to get around the static analysis tool, they will.

[hnnweb]

If you're wondering why there is a sudden uptick in the activity of this issue, it's because it has been linked on reddit, probably because it's both funny and sad and that makes for good reddit content. Anyhow, as you said, it's an arm race you cannot win with static analysis. If someone wants to get around the static analysis tool, they will.

And one of the top posts on HackerNews
Semgrep: JavaScript browser security concat bypass not detected

[jondubois]

That approach looks ugly. That said, it's not so different from what React and other front end frameworks are doing behind the scenes.

I'm not a big fan of these automated 'security tools' which output 99% false positives. For example raw-html-concat COULD be a security risk, but it's actually not a security risk in 99% of cases (for example if not using untrusted user-generated input).

Anything can be regarded as a vulnerability if you account for that 1% edge case where the tool is being misused.

[jdmarshall]

@jondubois There's two reactions to being questioned about html interpolation: Either a detailed description of how we architected it out, in which case they are right 99% of the time (not as in 1 out of 100 respondents are wrong, but as in 1 out of 100 call sites don't follow the rules they think all the code follows), or someone says that's not a problem and 100% of the time they're wrong and they just don't know it yet.

I got into a proper, full-on argument with a guy over SQL injection attacks in PHP, and not 20 hours later someone was using his server to run malware. I looked at the patch they did to fix the problem, and there were a handful of other spots in the same section of code that had the exact same antipattern as the couple queries they fixed. I just can't even fathom how such a person makes their way safely in the world. None are so blind as those who will not see.

Never trust the client. If you can't tell that the information came from the client, then don't trust anything. If you can't do either of those, then brother, are you ever in for it.

[jondubois]

I'm surprised that so many people gave thumbs down to my comment for merely stating computationally provable facts. There is a big difference between code being bad/ugly and code being a security vulnerability.

Security-wise, concatenating strings in this way is no different from using the newer template strings syntax which everyone uses in React... In any case, you need to sanitize the input before you embed it into the string.

You can just run these two JavaScript expressions to check for yourself:

let object = { toString: null }; let str = 'hello ' + object;

or

let object = { toString: null }; let str = `hello ${object}`;

Both will throw TypeError: Cannot convert object to primitive value
Both can potentially crash your app.

[nsweaney]

Jon, the reason it provides some risk, even if low, is because you may use it in a perfectly safe manner, but you have no idea what the next dev following you will do. Business requirements may change and the data that you thought was safe may suddenly be modified by untrusted users. Or some future developer may simple assume that you followed best practices and treated the data safely. Or business acquisitions may combine databases with code that wasn't properly sanitized. There are a million reasons why your "safe" use-case may become a vulnerability later on. I find these vulnerabilities often in apps that were never intended to be used how they ended up.

That's the point of best practices built on the principle of defense in depth. It's not about building protections for perfect situations, it's about building layers of protections so that when one fails, or something changes that you depended on, you're still okay. What you call a 1% edge case may still present risk to the organization. It may be small, and you may consider it a false positive, but it's still worth reviewing and considering the potential impact.

With that said, you're absolutely correct about the weakness of automated scanning tools and false positives. They shouldn't be used in a vacuum (as apparently the OP's company). Manual testing is just as important (or more so). But automated scanners can help ensure best practices and standardization so that manual testing is able to focus on more complex vulnerabilities.

[jondubois]

@nsweaney In most cases, no matter how many layers you have, all it takes is one bad line of code to bring the whole thing down. The best protection is high quality automated testing. Everything else is just bureaucracy. It makes no sense that some automated tool which knows nothing about your business domain can somehow find vulnerabilities.

I don't know what kind of code people write these days, but for my multiple open source projects over the past decade, I have never once seen a single correct vulnerability report from any automated tool - On the other hand, I've seen hundreds of false negatives which I was forced to address anyway because my upstream dependents kept raising issues about tool X raising a warning about dependency Y. I even recall at least one instance where the suggested fix would have introduced a real vulnerability into my project.

[jdmarshall]

@jondubois

The problem with code smells is that they blunt the power of human audits. It's something you don't necessarily pick up on unless you really watch people (or yourself) go through the process and trying to figure out where the waste occurs.

Code that looks wrong gets time spent on it, in perpetuity.