Hacker News Reader

Comment by tyingq | Hacker News Reader

The big missing feature for these kinds of tools is a workflow and relationship for dev teams to mark findings. Marking them as "false positive" or "only applies if these other conditions are true", or "yes, but we have a mitigation/exception". etc. A fast workflow that allows for less blockers, reduced noise and a focus on things that actually matter.

View on HN

Totally agree. I love the idea of SAST-in-CI, but I ran this on a handful of repos I manage (ranging from 40k-100k SLOC) and there were too many false positives for me want to add this as build-breaking criteria to our CI pipeline. Not unique at all to bearer in any way of course, as you point out, but still a real problem.

I suppose an alternative would be to not have this be a zero-sum part of CI, but maybe as a qualitative summary that gets autogenerated as part of the PR / code review process. The noise issue is still a real one as people will eventually ignore the noisy summaries or filter/whitelist them into relative oblivion.

I like the idea of "only applies if these other conditions are true". In all the false positives I encountered so far, if given the option I would be able to declaratively express when and when not to apply the rule. I'd even be ok with inline ignore comments to that end which, while not ideal, is something folks are already used to for other idioms like test coverage et al.

I’ve introduced the `absence` trigger that does that If express is present but helmet is missing then break.

Do you think that’d help achieve what you have in mind?

I think the design flaw in most of the problematic rules was from too simple of regex matching. Looking for a string pattern should be a clue to do some deeper analysis (maybe verify via AST), not necessarily to flag the string alone as security failure.

The rules do work on the AST but the current cookie rule is not as advanced as it could/should be. For example, we really should treat encryption as sanitizing the value.

We'll take another look at the rules with this in mind. If you are able to share the (rough) approach you take to build the cookie string it would help us to ensure we're covering the specific case(s) you have.

We need to open for configuration the filtering and prioritization logic that essentially does that today, but so you can apply your own logic.

I advise to start today by looking first only to critical alerts, with our scoring based on sensitive data impact that should be a good first step in triaging.

Btw if you have some exemple please share or even better write an issue, we’d be super happy to look at it and fine tune the rules.

It’s just a 1.0, we can do much better for sure :)

I'll cherry pick an example: the default cookie config rule(https://github.com/bearer/bearer/blob/main//pkg/commands/pro...).

We have many places where `cookie: <EncryptedString>` is used in our code and it triggers that rule. There are a few issues with this:

- Most of the expressions where we use that pattern are used to send a full encrypted cookie string. The use of `cookie` is not the name of a key in the cookie string, its the whole cookie.

- All of the data in the cookie string itself is encrypted and also sent over https. Just matching on a regex expression won't tell you this information without an accompanying AST to verify.

Notably, we're using hapi and not express but my notes above would still apply to some use cases in express as well. Its possible I am missing the actual value of that rule, but just matching on the expression is going to generate a ton of false positives.

Thanks for the feedback here; it is much appreciated :) I do know your point around catching encryption is more general than this example, but I’ve made a small improvement to the default cookie config rule regex to address one of the false positive cases mentioned https://github.com/Bearer/bearer/pull/754

This still generates the same false positive for me, in all of the previous repos I tested on.

Thanks for the report back; that's interesting. Perhaps I misunderstood your example. Feel free to write an issue if you like, and I can investigate further.

Workflow is coming with our Cloud offering, with all the cool integration you can think of as Jira or Slack.

On the "marking" part, we have two options that will be available super soon: 1) Directly in the code, by adding a special comment that will ignore findings. 2) In the Cloud, an ignore action will forever park an issue, even if it changes line etc. (smart fingerprinting applied). We can't really have that in the OSS since it's state-less.

Gitlab has a great security dashboard for this. It organizes the output of multiple tools in a place where you can discuss, triage, ignore or track an issue to resolve it.

https://docs.gitlab.com/ee/user/application_security/securit...

Also, super expensive, you need the $99 plan :) https://about.gitlab.com/pricing/

Integration with SCM is clearly a top priority for us, especially directly in PR. GitHub SARIF is a nice way to integrate third-party into their Dashboard, we're commited to it.

100% agree

(shameless plug, the product we are working on for the last 1.5 years aims to solve exactly that… either via a PR bot, slack / teams etc). Ping me (see profile for details) if it’s interesting.

Github actually has this feature (only for open source and enterprise IIRC) when there is a SARIF output

SARIF output is on our Roadmap btw!

Github code scanning is not so great from what we've heard so far, but also it's very expensive, you need to be on the Enterprise plan...

Reply on news.ycombinator.com