Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

First pass at annotations / ignore "known false-positives" #73

Closed
wants to merge 15 commits into from

7 participants

@daveworth

A much cleaner version of the previous commit (correctly merged with upstream changes I believe). This is definitely just the beginning but it seems useful at this point. What's the opinion of the Brakeman team?

@daveworth daveworth referenced this pull request
Closed

Annotations #72

lib/brakeman/checks.rb
@@ -31,6 +31,7 @@ def initialize options = { }
@template_warnings = []
@model_warnings = []
@controller_warnings = []
+ @ignored_warnings = nil
@presidentbeef Owner

This needs to be an array by default, otherwise it will be nil here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/brakeman/checks.rb
@@ -56,6 +57,17 @@ def add_warning warning
end
end
+ def filter_by_annotations(annotations)
+ ignored_warning_hashes = annotations.map { |a| a[:hash] }
+ @ignored_warnings = warnings.select { |w| ignored_warning_hashes.include? w.annotation_hash }
+
+ [@warnings, @template_warnings, @controller_warnings, @model_warnings].each do |warning_group|
+ warning_group.reject! { |w| ignored_warning_hashes.include? w.annotation_hash }
+ end
+
+ puts "Ignoring #{@ignored_warnings.size}"
@presidentbeef Owner

This may just be for debugging, but if not please use Brakeman.notify

Definitely a debug statement, will remove.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/brakeman/checks.rb
@@ -56,6 +57,17 @@ def add_warning warning
end
end
+ def filter_by_annotations(annotations)
+ ignored_warning_hashes = annotations.map { |a| a[:hash] }
@presidentbeef Owner

Can we use a Set for this?

Edit: Sorry, I mean can use a set for ignored_warning_hashes instead of an array.

It probably could be. What would the win be in that case?

Let me retract that... I totally see the win! Doing just that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@presidentbeef

Hey Dave, do you have the time to update this to work with the new reporting code?

@daveworth

I can probably make some time but haven't seen the new reporting code. Since I think annotations are valuable I will definitely look into this ASAP.

@daveworth

Just a note, during a long flight tomorrow I hope to nail down the port to the new reporting code. Sorry for the delays!

Dave Worth added some commits
lib/brakeman/warning.rb
@@ -163,4 +165,20 @@ def to_json
JSON.dump self.to_hash
end
+
+ def to_annotation
+ clean_annotation.merge({:digest => self.annotation_digest, :note => ""})
+ end
+
+ def clean_annotation
+ self.to_hash.merge :line => line
@presidentbeef Owner

Adding the :line value shouldn't be necessary anymore, right?

Is it added elsewhere? I chose to add it here, after the digest was calculated, so that other changes that might move an annotated warning would not affect the value of the digest, but would still be alerted during annotation calculation. Does that make sense?

@presidentbeef Owner

It's added in #to_hash (this changed since you first wrote the code). I agree with the intention that the digest should exclude the line number.

oh, in that case clearly you're right! I'll patch this up ASAP.

Actually, in that case there's another bug. looking below in #annotation_digest we call #clean_annotation which includes the line number! :bomb:

Actually, now recalling, maybe I was attempting to include the line number in case surrounding context made the original annotation moot because mitigations were put in around it! I think that actually makes more sense (sorry, it's been a while!)

Either way, if it's added in #to_hash maybe we should just call it instead of #clean_annotation?

@presidentbeef Owner

Not sure...I interpreted it as not including the line number in the annotation digest.

I'm not really certain which is preferred. I think it is better to ignore the line number, because the case where adding a line to the beginning of the file invalidates all the annotation digests seems bad. But so does missing a potential vulnerability because someone copy-pasted code into a slightly different context.

This is why this is a hard problem!

We were just talking about this today! My vote is that the line number should not be included. I see the clobbering of findings as an edge case. Maybe when generating the annotations file, we can detect if results are being clobbered. (Will add a note to the appropriate line)

soapbox
Copying/pasting code is the 8th deadly sin. The extended version of Se7en included scenes of a coder, but I won't spoil that surprise.

off soapbox
I too sin in this fashion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/brakeman/report.rb
@@ -675,4 +721,20 @@ def load_and_render_erb file, bind
template = ERB.new(content)
template.result(bind)
end
+
+ def to_annotation
+ warnings = @checks.all_warnings
+ warnings.map { |warning| warning.to_annotation }.to_yaml

This presumes that we are taking line numbers out of the hash...

maybe we can add a war ning that the warning has already been added to the list of annotations?

uniq = []
warnings.each do |warning|
  if uniq.include? warning
    # WARN!!!! log? comment in annotation file? something!
  else 
   uniq << warning
end

There's gotta be moar rubier way to do this, but I am out of brainpower

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@presidentbeef

Finally spending some time on this (sorry @daveworth!).

I only have one big concern, and that's the brittleness of the digest. The YAML output appears to be different based on the order of the keys, which in Ruby 1.8 may vary. Even in 1.9 we have to be careful not to do anything that might change the insertion order of the keys.

It also locks us in to the current set of information included in the warnings. We can't change Warning#to_hash without breaking everyone's false positive list.

@presidentbeef

I just noticed the file part of the annotations is the full path. This should probably be relative.

I think we can leave the line numbers in for now; I have an idea of how to improve this in the future.

One small thing: this should work when specifying an extension in the file name (yaml or annotation?) instead of needing to use the -f option.

@daveworth

@presidentbeef

Let's see if I can address a bunch of the above concerns here quickly:

  • Line numbers: I included them to force the annotation to be brittle. My (maybe very flawed reasoning) was that something like
Model.where("id = #{params[:inject_me]}")

which had an annotation attached to it would then force a reinvestigation of the annotation when it was changed to the following:

params[:inject_me] = sanitize_param_against_injection(params[:inject_me])
Model.where("id = #{params[:inject_me]}")

clearly the above is a) contrived, b) a horrible idea (since it will never work) and c) still somewhat brittle as removing a line and adding the santizer would not break the digest based on line number.

  • Hash ordering: Yep, you can tell that really I do too much in 1.9. What we could do is sort the hash by a sorting function we control before exporting to YAML. That would guarantee consistent ordering.

  • Full vs. relative paths of source files. Both will break if you move app/models/foo.rb to app/models/badly_named_classes/foo.rb (via s/class Foo/class BadlyNamedClassees::Foo/). What advantage does the relative path in the digest provide?

  • Totally dig the idea that -o foo.annotation or -o annotations.yaml would output an annotations file. My choice in not doing this was based on the idea that annotations are an edge-case and not part of a standard workflow. For some reason both extensions feel ambiguous. We probably have no plans to output YAML in the future but auto-detecting it now and outputting annotations that way may be the wrong thing so perhaps the .annotations extension is better?

@daveworth

Just a follow-up, I can definitely cut some work time to work on getting this into mainline in the future since part of my job is contributing to open-source security (ruby-specific) tools! If we work out a little bit of a roadmap we can get this nailed down.

@presidentbeef
  • Not sure the line number matters much in the long run, either way. There are pros and cons for both approaches. (In your example, the code should be detected as different, but I get your point.)
  • Not sure how sorting the hash would work, but seems like a reasonable solution if it does.
  • Relative paths are useful if you want to be able to move the annotations to a different machine/user or other situations where paths might change, like a CI system.
  • annotation or annotations works for me, as long as it matches the -f option
@daveworth

@presidentbeef

  • Regarding hash sorting, we could do something really stupid and deterministic, or something more clever and deterministic, the key being deterministic. The sorting would be a function of the hash-keys themselves, and can literally be a weight assigned to the keys in the order which we expect them, then maybe lexicographically sorting those keys we don't expect (which shouldn't be there). That should be 1.8.x and 1.9.x compliant right?

  • That's a great point re: relative paths. I was thinking about refactorings not about different developers. A++

  • Ok, maybe .annotations for auto-detection make sense.

@oreoshake

Seems like the sorting can be arbitrary, as long as it's consistent.

@daveworth

@oreoshake totally, which is why I started with "stupid" :blush:

@presidentbeef

I'm fine with the standard sort on the keys (have to convert to strings for 1.8), all we care about is consistent YAML so we get consistent digests.

Won't you have to convert to something other than a hash before converting to YAML? A 1.8 hash will not maintain any kind of ordering.

@presidentbeef

Just as an FYI, I'd like to get the next release of Brakeman out on Monday. I'd like this to be included, but it's alright if it's not ready.

@daveworth

I would love that. At this point, what changes do we want included in that release? Off the top of my head:

  • Constant sorting of keys to make YAML consistent on 1.8.x
  • relative paths
  • do we include line numbers or no?
  • .annotations autodetection of format.

Is that it?

@oreoshake

I vote for not including line numbers :)

@presidentbeef

That sounds right to me. I'm also in favor of excluding line numbers from the digest as the slightly better approach.

Dave Worth added some commits
Dave Worth Add support for `.annotations` extension
If the output file has the `.annotations` extension, automatically use the
annotation output format
132ce36
Dave Worth no longer include line numbers in annotations dc331d3
Dave Worth report with relative paths
* Make annotations report relative paths like HTML report does

* Extract relative path cleaning and remove bogus leading slash on filenames
  since we are almost never talking about `/config/routes.rb` but rather
  `RAILS_ROOT/config/routes.rb` or more simply `config/routes.rb`
7cf1f3f
@daveworth

All of the requested features are in place. I removed line numbers from the annotation file entirely as if they aren't being digested they certainly shouldn't be in the annotations because as things move around that will get confusing. We can change that if that wasn't the right reasoning.

@presidentbeef

Hmmm...I can see that as being confusing, but won't it be hard to initially correlate annotations with warnings if you don't have line numbers? I guess you would just have to match the code?

@daveworth

I can add the lines back in. Should I? I will happily do so.

@oreoshake

Yeah I think it would be best to have the line numbers included in the yaml file, but not use it for the digest.

And since it's friday, an emoji is necessary :surfer:

@presidentbeef

Yes, let's go with that. :space_invader:

lib/brakeman/warning.rb
@@ -163,4 +165,27 @@ def to_json
JSON.dump self.to_hash
end
+
+ def to_annotation
+ self.to_hash.merge({:digest => self.annotation_digest, :note => ""})
+ end
+
+ def clean_for_annotation
+ h = self.to_hash
+ h.delete(:line)
+ h
+ end
+
+ def annotation_digest
+ digested = ""
+ clean_for_annotation.keys.map(&:to_s).sort.each do |k|
+ digested << k << self.to_hash[k.to_sym].to_s
@presidentbeef Owner

Very minor thing, but can we store self.to_hash in a local variable so it doesn't need to create a new hash each loop?

Not minor at all! Great call! That was clearly early morning coding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/brakeman/report.rb
@@ -675,4 +721,27 @@ def load_and_render_erb file, bind
template = ERB.new(content)
template.result(bind)
end
+
+ def to_annotation
+ warnings = @checks.all_warnings
+ warnings.map do |warning|
+ warning.file = warning_file_to_relative_path(warning.file)
+ warning.to_annotation
+ end.to_yaml
+ end
+
+ def load_annotations annotations_file
+ annotations_file ||= ""
+
+ [File.expand_path(annotations_file), File.expand_path("./.brakeman_annotations.yaml")].each do |f|
+ if File.exist? f and not File.directory? f
+ #notify "[Notice] Using annotations in #{f}"
@presidentbeef Owner

Just noticed this is commented out. Let's use it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/brakeman/checks.rb
@@ -57,6 +59,15 @@ def add_warning warning
end
end
+ def filter_by_annotations(annotations)
+ ignored_warning_digests = Set.new(annotations.map { |a| a[:digest] })
+ @ignored_warnings = warnings.select { |w| ignored_warning_digests.include? w.annotation_digest }
@presidentbeef Owner

This should be all_warnings.select.... Sorry for the terrible API :cry:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/brakeman/options.rb
@@ -129,7 +129,7 @@ def get_options args, destructive = false
opts.on "-f",
"--format TYPE",
- [:pdf, :text, :html, :csv, :tabs, :json],
+ [:pdf, :text, :html, :csv, :tabs, :json, :annotation],
@presidentbeef Owner

The -f and -o options should be consistent, so I think this should be :annotations

yeah, good call. Sorry, this is my fault after spreading the dev over such a large window

@presidentbeef Owner

Oops, thought this was straight-forward but now :annotations needs to be supported with Report#to_annotations, etc. :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@daveworth

Man, sure would be nice to have a test-suite failing on these. You don't happen to do you?

@presidentbeef

No, but I'm certainly thinking about it now. :( It actually took me a while to figure out why -f annotations wasn't working, but also wasn't raising any errors.

@presidentbeef presidentbeef commented on the diff
lib/brakeman.rb
@@ -262,12 +266,15 @@ def self.scan options
end
tracker.run_checks
+ reporter = tracker.report
+ reporter.filter_by_annotations(options[:annotations_file]) if options[:annotations_file]
+
if options[:output_files]
notify "Generating report..."
@presidentbeef Owner

Needs to be Brakeman.notify...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@oreoshake

Justin and I were playing with this today, it doesn't appear to work due to relative/absolute paths. The annotations file generates relative paths where the comparison using the absolute path. Also, the warning.file = warning_file_to_relative_path(warning.file) would affect the other reporting formats. What about updating def to_hash -> def to_hash(options = {relative_path => false}) and passing in the condition when generating hashes for annotations?

@presidentbeef

I made a couple quick tests on the annotations_test branch.

@grosser

how about a simple # brakeman-ignore: <explanation> <-> so I can just do it at the line level and do not need to have some configuration which get's out of date/sync ?

@daveworth

@grosser would that be a comment then? What happens when another dev changes that line and adds a horrible remote vuln and it's ignored still?

@oreoshake

This is an interesting idea. I see a lot of tricky edge cases but I would definitely like to discuss more. Just quick thoughts:

  • I like it. Smpfy. However, it's slightly more difficult to audit. Could add a brakeman --ignored-lines that will just grep and spit back what would be a centralized config
  • Mark at the sink, not the source, right?
  • Comments in views are not popular :(
  • We should restrict to class of vulns, not just blanket "safe"
  • Warn on annotations that have no effect (like this FP was fixed in the most recent version, delete yo comment!)
@daveworth

@oreoshake interesting. If you do it inline, which might be simpler than the previous YAML based approach, then lots of simplicity may arise. I like that.

What worries me is that if you do it at the sinks (which is when the vulnerability is triggered) then you mask the actual vulnerable behavior in some cases. Sure, in the case where there is a vulnerable scope or redirect which is protected by some strong conditional as others have mentioned in the past, this is exactly the case where this may make sense, but I think this is an more uncommon use-case right?

@oreoshake

Are you saying we should prefer marking the source? If we do this, we could potentially mask a lot of warnings. This is mitigated by requiring the user to specify the class of warning, but I still feel a little uncomfortable here. I will think on this and come up with concrete examples or determine that my fear is unfounded.

I generally try to do things at the sink, especially when one param could be used in 20 different contexts. And the strategy so far has been "avoid doing anything that might potentially hide a warning at all costs", which I like. And I'm biased because I'm of the "let other tools dedup" mentality. But @daveworth's (and others') use case would clearly benefit from something like this.

This brings up another interesting point: if we do require the specification of a warning type, how do you handle multiple "ignores"? This will arise in both approaches

# brakeman-ignore: <SQLi>: <explanation>
# brakeman-ignore: <XSS>: <explanation>
# brakeman-ignore: <Redirect>: <explanation>
@mything = params[:donkey]

<%=
# brakeman-ignore: :
# brakeman-ignore: :
link_to @mything, @mything
%>

@matt-glover

I definitely agree you want to restrict to the specific static analysis rule that is violated rather than applying a blanket "safe code" attribute. The "this suppression is no longer valid" warning is also key.

I tend to prefer applying source-level rule suppression at the method level over the line level if possible for a few reasons (probably more personal than pragmatic to be honest):

  • I find it easier to track what rules are suppressed if I can always look in one place and expect to find that there. The same way you might include a note about unusual behavior in a comment block for a method.
  • Often times when I have code that violates a rule but I know it is okay due to some other mitigation I will have several instances of that problem all within the same method and I would rather not spam my code with multiple suppression notes.

I think ultimately it is up to the development team to keep tabs on their static analysis suppressions and, if necessary, remove the suppressions when relevant code is changing.

@presidentbeef

I don't like the idea of comments as annotations and disagree on it being simpler. A single file with all the false positive information in it is much easier to manage, for both the user and Brakeman. Not to mention any external tools.

By the way, this approach (comments) was already implemented: #6

@daveworth

@oreoshake clearly marking the source is dangerous but if you're willing to have say, a scope with a SQLi in it just sitting around, it feels like a decision has been made to have that. Devs are being trained on correct behavior, it is well documented, etc. and thus marking it there makes sense. Otherwise you may have annotations all over your source files which I hate.

This is one case where an external annotation file may be nice as it is auto-generated by Brakeman and in one place to be checked. Also we can throw warnings every time an alert is squelched by such an annotation if we want.

@daveworth

@presidentbeef I definitely do not prefer this method at all but it is somewhat conceptually simpler. That's all I meant. Now, perhaps, it is time for me to get some work done on Brakeman again and implement this? I think so. At least then we can compare contrast with something concrete even if it isn't used in the end.

@grosser

@daveworth yep a comment, there is only so much room in one line, so the chances are pretty slim / I'd be willing to take them, also it could be more specific like # breakman ignore mass-assignment

@grosser

the chances of having multiple warnings on one line are pretty slim -> let's not worry about it, if it happens you can still just ignore everything and write a description like "we need to do funky sql and mass assignment to support xyz"

@matt-glover

@daveworth I am not sure I understand how annotations in source are any more dangerous than annotations in an external file referencing that same source.

@oreoshake

Conceptually simpler for the end user was all I was getting at too. I do think one off comments are easier to grok than a (hopefully) small yaml file.

Not that Java is should be the golden standard, but they migrated to annotations from configuration files a long time ago, and while I haven't done any Java for a few years, I don't think anyone looked back? That said, 85 annotations in a file is obviously horrible.

@grosser

I'm not a big fan of central config files since they would get outdated + nobody takes care of them -> code get's refactored -> builds break
The brakeman ignore is also a nice warning for devs that this might look evil/bad but is there for a reason or can be ignored.

@daveworth

@grosser if the build breaks, it should probably break while you're running the test-suite locally right? Are you thinking about the CI case for test-suites that take hours to run? I like to hope we can mostly avoid the latter case. Break sooner is always my goal.

@matt-glover

@oreoshake I've seen the same trend over in the C# world. Similarly the .NET approach is not the gold standard by any stretch either.

@matt-glover

@grosser I think that is why I tend to prefer in-code over external file solutions. I find it easier to understand what is going on when something relevant to the code lives right near that code.

@presidentbeef

This is how I see it.

Central config:

:+1: Easy to generate
:+1: Easy to parse (YAML.load versus...I don't know what for comments. Regex?)
:+1: Easy to manipulate by external tools (CI, version control, etc.)
:+1: Easy to delete if one decides to stop using Brakeman
:+1: Easy to understand semantics - one annotation in the file corresponds directly to warning output
:-1: Not as easy to manipulate manually (at least I hate dealing with YAML)
:ok_hand: Not as closely tied to code (good and bad, IMO)
:-1: Apparently out of style?

Comments in code:

:-1: Have to remember "Brakeman annotation syntax," even if simpler than YAML
:-1: Hard to parse out of code and associate with warnings that should be ignored
:-1: Hard to manipulate/read with external tools
:-1: Hard to remove later
:-1: Semantics are not as clear - what annotations hide which warnings?
:-1: Clutters up codebase over time
:+1: Right there in the code, so easy to see
:+1: Nice documentation

Either could get out of sync with the code, so I don't see that as an advantage of either.

@oreoshake

Just want to throw this out there: either way, the first thing one should do when declaring a false positive is open an issue on Github :smiley_cat:

While use cases may vary wildly, thus influencing the conversation, I think the best way to handle false positives is to fix them in the tool :)

For me, the false positive SLA for warnings/LOC for medium/high confidence warnings should be < 1% (made up metric to reflect ~ 0 FPs)

@grosser

btw most of our false-positives are "Admin only controller, no need to verify mass-assignment"

@matt-glover

Central config:
...
:+1: Easy to parse (YAML.load versus...I don't know what for comments. Regex?)

I think this hits at a core issue. There is no real standard for true in-code annotations in Ruby and brakeman probably does not want to be the one trying to invent such a standard.

:+1: Easy to manipulate by external tools (CI, version control, etc.)

Do you have a use case handy where external tools need to modify or control suppressions separately from the code? Seems like there is already a flag in this request to turn on annotations when you want them.

:+1: Easy to delete if one decides to stop using Brakeman

Also it would probably be easier to convert if one decides to switch from brakeman to some theoretical future tool as well.

:+1: Easy to understand semantics - one annotation in the file corresponds directly to warning output

It is still not clear how an external file is any more clear than an in-code annotation with most of the same attributes. Are you simply arguing it is easier to read in the external file?

Comments in code:
...
:-1: Have to remember "Brakeman annotation syntax," even if simpler than YAML

You still have to learn brakeman's annotation syntax that happens to be YAML formatted in a central config so this is sort of a wash.

:-1: Hard to manipulate/read with external tools

Is it actually harder for an external tool to scan for a custom formatted-comment from a ruby file instead of a YAML file? I guess you have to cover each file with in-code annotations but otherwise I do not see a huge barrier here.

:-1: Clutters up codebase over time

To me this is like saying normal comments clutter up a codebase over time.

With all that said the lack of true annotations in Ruby is a strong enough argument, to me, to keep it external for now.

@presidentbeef

It is still not clear how an external file is any more clear than an in-code annotation with most of the same attributes. Are you simply arguing it is easier to read in the external file?

No, I mean the tool (Brakeman) must be able to determine what an annotation means in terms of associating an annotation with a warning to ignore. If the inline comment contains the same information as the proposal in this pull request, then you are correct that it is equal. However, I don't think anyone would want to write comments containing as much info as the generated annotations have.

This is one of those cases where looking at it as a human it seems obvious ("this comment must refer to this ActiveRecord query"), but having a tool do the same is complicated. Especially when you have to write a separater parser for it (RubyParser ignores comments as far as I know).

You still have to learn brakeman's annotation syntax that happens to be YAML formatted in a central config so this is sort of a wash.

Not really, annotations are generated by Brakeman itself. Hopefully no hand-editing necessary except copy-paste.

Do you have a use case handy where external tools need to modify or control suppressions separately from the code? Seems like there is already a flag in this request to turn on annotations when you want them.

...

Is it actually harder for an external tool to scan for a custom formatted-comment from a ruby file instead of a YAML file? I guess you have to cover each file with in-code annotations but otherwise I do not see a huge barrier here.

I know this is a bit theoretical, since there isn't a concrete tool that already exists. Consider having a GUI for Brakeman. This is YAML.load and YAML.dump versus scanning every file in an application and somehow building up the same data structures. And then what if you want to change the annotations? Find and replace in the app files? Just seems messy in comparison.

@grosser

let's get this merged then!

@brynary

Late to the party, but wanted to chip in a few thoughts:

  • I think Brakeman should use project-relative paths across the board, in all the output formats, which is not currently the case. This was mentioned in this thread, but I wonder if that change should be made everywhere as a prerequisite to this. (I've avoided it so far because it's potentially breaking for people who depend on Brakeman output.) @presidentbeef -- WDYT?

  • At a high level, the terminology introduced here is a bit confusing. The formatter is called "annotations", but it seems more like an ignore file? Annotations makes me think "inline comments", where this feels more like a YAML output or a .brakeman_ignore.yml concept. I'm also not a fan of the subtle difference between .brakeman_annotations.yml and brakeman_annotations.yml (leading period).

Would it work to call the output formatter simply "yml", and then have a default of using .brakeman_ignore.yml as an exclude?

  • Finally, I wonder about the resiliency of the fingerprints. Specifically, they are coupled to the copy Brakeman uses to describe the issues. This means we can't improve the copy (warning names, descriptions, etc.) without breaking fingerprints. The only way I see around this is extending the checks to pass in a more stable identifier for the issue, or reducing the fields in the fingerprint (may have adverse affects). Thoughts?
@grosser

:+1: on # 1 otherwise committing the ignore is useless + # 2 makes more sense imo

@presidentbeef

Definitely relative paths for "annotations" or "ignore" files (discussion way up above). Relative paths for JSON is okay (but would definitely need to be a 2.0 change). HTML, doesn't matter, already displaying relative paths. For tabs, can't do, as I'm pretty sure Jenkins needs full paths. CSV, I don't care.

I suppose it makes some sense to have the "annotations" be just another report type. But, unlike other reports, it needs to be read as input as well. I'm wondering if we can combine it with the Brakeman config file?

I'm also very concerned with the brittleness of the fingerprints. I think the fingerprint should be generated with the extra information available inside Brakeman - for example, using the Sexp instead of the string of the code, adding some context information, etc. I try not to change the messages because of this issue, but it may not be entirely avoidable.

@shedd

@presidentbeef from your presentation at OWASP (http://www.youtube.com/watch?v=Ivc5Sj0nj2c), it sounds like you're already handling false positives in an automated fashion at Twitter. How do you have this setup in SADB?

@presidentbeef

We don't, actually. In SADB the "false positive" button sends an email to the security team :) We generally believe in fixing false positives: either in the application code or in Brakeman. Also, we ignore low confidence warnings by default. That leaves us with very few false positives overall.

The next version of Brakeman will include more information about warnings to make it easier to build tools that can ignore false positives.

@presidentbeef

If everyone would kindly head over to #368, I'd be interested in your feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 5, 2012
  1. Change from Ruby 1.9 to 1.8 style hash rockets

    Dave Worth authored
  2. mixup bug where all annotations were getting the same digest

    Dave Worth authored
    * s/hash/digest since digest is so loaded in the Ruby world
  3. patchup strange encoding issues on ruby 1.9(.3?) in exporting yaml

    Dave Worth authored
    * to_yaml insisted on exporting binary rather than string data.  The google+stack-overflow indicated it was a UTF-8 export issue
  4. update reporting code to not use Ruport!

    Dave Worth authored
Commits on Jul 20, 2012
  1. Add support for `.annotations` extension

    Dave Worth authored
    If the output file has the `.annotations` extension, automatically use the
    annotation output format
  2. no longer include line numbers in annotations

    Dave Worth authored
  3. report with relative paths

    Dave Worth authored
    * Make annotations report relative paths like HTML report does
    
    * Extract relative path cleaning and remove bogus leading slash on filenames
      since we are almost never talking about `/config/routes.rb` but rather
      `RAILS_ROOT/config/routes.rb` or more simply `config/routes.rb`
Commits on Jul 23, 2012
  1. use notifcation that an annotation is in use.

    Dave Worth authored
  2. s/warnings/all_warnings/

    Dave Worth authored
  3. s/annotation/annotations/ in formats

    Dave Worth authored
This page is out of date. Refresh to see the latest.
View
26 README.md
@@ -115,6 +115,32 @@ To compare results of a scan with a previous scan, use the JSON output option an
This will output JSON with two lists: one of fixed warnings and one of new warnings.
+## Using annotations to ignore false positives
+
+Brakeman can now produce an "annotation" output format via `-f annotation`. The output of this format is a YAML file which marks up the various warnings produced in a brakeman run. The intention of this format is to extract the various warnings your security team has identified as technically a vulnerability but one that will not affect the system's integrity or that the service owner has accepted risk on (for you ITIL types). The general workflow for using annotations is as follows:
+
+1. Run brakeman with `-f annotation -o brakeman_annotations.yaml` options.
+2. Extract the warnings you wish to ignore as false-positives from `brakeman_annotations.yaml` into `.brakeman_annotations.yaml` noting the differing leading periods. The `-A` flag defaults to using the `.brakeman_annotations.yaml` filename but another could just as easily be used. Also make sure the leading three dashes (part of the YAML markup) remain in place. Do not rearrange the lines of the YAML encoded hash before the `hash` attribute but you may edit everything occurring after the `hash` attribute. For instance you might edit the note attribute to indicate why you have chosen to ignore that mass-assignment warning we've created for you.
+
+```yaml
+ ---
+ - :warning_type: Mass Assignment
+ :message: Unprotected mass assignment
+ :file: /Users/mrdev/Documents/my/app/controllers/vulernablity_controller.rb
+ :code: Vulnerability.new(params[:vulnerability])
+ :location:
+ :type: :method
+ :class: :VulnerabilityController
+ :method: :not_create
+ :confidence: High
+ :line: 51
+ :hash: 29d2d5ec2b388060c746d6901e477ef9
+ :note: 'We ignore this because this action does not have a route'
+```
+3. Run Brakeman with the `-A <filename>` option where the filename is the name of your new annotations file if you haven't used the default name. The warnings from your annotation file should be ignored with a note to that effect in the report.
+
+For best results use the `--separate-models` option to brakeman. That way you can ignore a single mass-assignment vulnerability (for example) and not ignore _all_ mass-assignments vulnerabilities which would be The Wrong Thing :TM:
+
# Warning information
See WARNING\_TYPES for more information on the warnings reported by this tool.
View
39 lib/brakeman.rb
@@ -134,20 +134,22 @@ def self.get_output_formats options
end
if options[:output_format]
[
- case options[:output_format]
- when :html, :to_html
- :to_html
- when :csv, :to_csv
- :to_csv
- when :pdf, :to_pdf
- :to_pdf
- when :tabs, :to_tabs
- :to_tabs
- when :json, :to_json
- :to_json
- else
- :to_s
- end
+ case options[:output_format]
+ when :html, :to_html
+ :to_html
+ when :csv, :to_csv
+ :to_csv
+ when :pdf, :to_pdf
+ :to_pdf
+ when :tabs, :to_tabs
+ :to_tabs
+ when :json, :to_json
+ :to_json
+ when :annotation, :to_annotation
+ :to_annotation
+ else
+ :to_s
+ end
]
else
return [:to_s] unless options[:output_files]
@@ -163,6 +165,8 @@ def self.get_output_formats options
:to_tabs
when /\.json$/i
:to_json
+ when /\.annotations/
+ :to_annotation
else
:to_s
end
@@ -262,12 +266,15 @@ def self.scan options
end
tracker.run_checks
+ reporter = tracker.report
+ reporter.filter_by_annotations(options[:annotations_file]) if options[:annotations_file]
+
if options[:output_files]
notify "Generating report..."
@presidentbeef Owner

Needs to be Brakeman.notify...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
options[:output_files].each_with_index do |output_file, idx|
File.open output_file, "w" do |f|
- f.write tracker.report.send(options[:output_formats][idx])
+ f.write reporter.send(options[:output_formats][idx])
end
notify "Report saved in '#{output_file}'"
end
@@ -275,7 +282,7 @@ def self.scan options
notify "Generating report..."
options[:output_formats].each do |output_format|
- puts tracker.report.send(output_format)
+ puts reporter.send(output_format)
end
end
View
13 lib/brakeman/checks.rb
@@ -1,5 +1,6 @@
require 'thread'
require 'brakeman/differ'
+require 'set'
#Collects up results from running different checks.
#
@@ -9,7 +10,7 @@
class Brakeman::Checks
@checks = []
- attr_reader :warnings, :controller_warnings, :model_warnings, :template_warnings, :checks_run
+ attr_reader :warnings, :controller_warnings, :model_warnings, :template_warnings, :ignored_warnings, :checks_run
#Add a check. This will call +_klass_.new+ when running tests
def self.add klass
@@ -32,6 +33,7 @@ def initialize options = { }
@template_warnings = []
@model_warnings = []
@controller_warnings = []
+ @ignored_warnings = []
@checks_run = []
end
@@ -57,6 +59,15 @@ def add_warning warning
end
end
+ def filter_by_annotations(annotations)
+ ignored_warning_digests = Set.new(annotations.map { |a| a[:digest] })
+ @ignored_warnings = all_warnings.select { |w| ignored_warning_digests.include? w.annotation_digest }
+
+ [@warnings, @template_warnings, @controller_warnings, @model_warnings].each do |warning_group|
+ warning_group.reject! { |w| ignored_warning_digests.include? w.annotation_digest }
+ end
+ end
+
#Return a hash of arrays of new and fixed warnings
#
# diff = checks.diff old_checks
View
6 lib/brakeman/options.rb
@@ -129,7 +129,7 @@ def get_options args, destructive = false
opts.on "-f",
"--format TYPE",
- [:pdf, :text, :html, :csv, :tabs, :json],
+ [:pdf, :text, :html, :csv, :tabs, :json, :annotations],
"Specify output formats. Default is text" do |type|
type = "s" if type == :text
@@ -161,6 +161,10 @@ def get_options args, destructive = false
options[:output_files].push(file)
end
+ opts.on "-A", "--use-annotations FILE", "Use annotations file" do |file|
+ options[:annotations_file] = file
+ end
+
opts.on "--separate-models", "Warn on each model without attr_accessible" do
options[:collapse_mass_assignment] = false
end
View
73 lib/brakeman/report.rb
@@ -35,11 +35,13 @@ def initialize tracker
@element_id = 0 #Used for HTML ids
@warnings_summary = nil
@highlight_user_input = tracker.options[:highlight_user_input]
+ @ignored_summary = nil
end
#Generate summary table of what was parsed
def generate_overview html = false
warnings = all_warnings.length
+ ignored_warnings = checks.ignored_warnings
if html
load_and_render_erb('overview', binding)
@@ -50,13 +52,14 @@ def generate_overview html = false
t.add_row ['Templates', number_of_templates(@tracker)]
t.add_row ['Errors', tracker.errors.length]
t.add_row ['Security Warnings', "#{warnings} (#{warnings_summary[:high_confidence]})"]
+ t.add_row ['Ignored Warnings', ignored_warnings.length]
end
end
end
#Generate table of how many warnings of each warning type were reported
def generate_warning_overview html = false
- types = warnings_summary.keys
+ types = warnings_summary.keys | ignored_summary.keys
types.delete :high_confidence
if html
@@ -316,6 +319,31 @@ def generate_templates html = false
end
end
+ def generate_ignored_notes html = false
+ if checks.ignored_warnings
+ warnings = []
+ checks.ignored_warnings.each do |warning|
+ annotation = @annotations.find{|a| a[:digest] == warning.annotation_digest}
+ if annotation[:note] && annotation[:note].length > 0
+ warnings << {"Warning" => "#{warning.warning_type} (#{File.basename(warning.file)}:#{warning.line})",
+ "Note" => annotation[:note] }
+ end
+ end
+
+ if warnings.length > 0
+ if html
+ load_and_render_erb("ignored_notes", binding)
+ else
+ Terminal::Table.new(:headings => ['Warning', 'Note']) do |t|
+ warnings.each do |warning|
+ t.add_row [warning["Warning"], warning["Note"]]
+ end
+ end
+ end
+ end
+ end
+ end
+
#Generate HTML output
def to_html
out = html_header <<
@@ -340,6 +368,7 @@ def to_html
out << generate_controller_warnings(true).to_s
out << generate_model_warnings(true).to_s
out << generate_template_warnings(true).to_s
+ out << generate_ignored_notes(true).to_s
out << "</body></html>"
end
@@ -381,6 +410,9 @@ def to_s
res = generate_template_warnings
out << "\n\nView Warnings:\n\n" << truncate_table(res.to_s) if res
+ res = generate_ignored_notes
+ out << "\n\nIgnored Warnings Notes:\n\n" << truncate_table(res.to_s) if res
+
out << "\n"
out
end
@@ -490,6 +522,15 @@ def warnings_summary
@warnings_summary = summary
end
+ def ignored_summary
+ return @ignored_summary if @ignored_summary
+ summary = Hash.new(0)
+ checks.ignored_warnings.each do |warning|
+ summary[warning.warning_type.to_s] += 1
+ end
+ @ignored_summary = summary
+ end
+
#Escape warning message and highlight user input in text output
def text_message warning, message
if @highlight_user_input and warning.user_input
@@ -545,7 +586,7 @@ def with_context warning, message
message
end <<
"<table id='#{code_id}' class='context' style='display:none'>" <<
- "<caption>#{(warning.file || '').gsub(tracker.options[:app_path], "")}</caption>"
+ "<caption>#{warning_file_to_relative_path(warning.file)}</caption>"
unless context.empty?
if warning.line - 1 == 1 or warning.line + 1 == 1
@@ -668,6 +709,11 @@ def number_of_templates tracker
Set.new(tracker.templates.map {|k,v| v[:name].to_s[/[^.]+/]}).length
end
+ def filter_by_annotations annotations_file
+ load_annotations annotations_file
+ @checks.filter_by_annotations(@annotations)
+ end
+
private
def load_and_render_erb file, bind
@@ -675,4 +721,27 @@ def load_and_render_erb file, bind
template = ERB.new(content)
template.result(bind)
end
+
+ def to_annotation
+ warnings = @checks.all_warnings
+ warnings.map do |warning|
+ warning.file = warning_file_to_relative_path(warning.file)
+ warning.to_annotation
+ end.to_yaml
+ end
+
+ def load_annotations annotations_file
+ annotations_file ||= ""
+
+ [File.expand_path(annotations_file), File.expand_path("./.brakeman_annotations.yaml")].each do |f|
+ if File.exist? f and not File.directory? f
+ notify "[Notice] Using annotations in #{f}"
+ @annotations = YAML::load_file f
+ end
+ end
+ end
+
+ def warning_file_to_relative_path(warning_file)
+ (warning_file || '').gsub(tracker.options[:app_path], "").gsub(/^\//, '')
+ end
end
View
13 lib/brakeman/templates/ignored_notes.html.erb
@@ -0,0 +1,13 @@
+<p>Ignored Warnings Notes</p>
+<table>
+ <tr>
+ <th>Warning</th>
+ <th>Note</th>
+ </tr>
+<% warnings.each do |warning| %>
+ <tr>
+ <td><%= warning["Warning"] %></td>
+ <td><%= warning["Note"] %></td>
+ </tr>
+<% end %>
+</table>
View
26 lib/brakeman/warning.rb
@@ -1,3 +1,5 @@
+require 'digest/md5'
+
#The Warning class stores information about warnings
class Brakeman::Warning
attr_reader :called_from, :check, :class, :confidence, :controller,
@@ -163,4 +165,28 @@ def to_json
JSON.dump self.to_hash
end
+
+ def to_annotation
+ self.to_hash.merge({:digest => self.annotation_digest, :note => ""})
+ end
+
+ def clean_for_annotation
+ h = self.to_hash
+ h.delete(:line)
+ h
+ end
+
+ def annotation_digest
+ digested = ""
+ clean_annotation = clean_for_annotation
+ clean_annotation.keys.map(&:to_s).sort.each do |k|
+ digested << k << clean_annotation[k.to_sym].to_s
+ end
+
+ digest = Digest::MD5.hexdigest(digested)
+ if RUBY_VERSION >= "1.9"
+ digest.force_encoding("UTF-8")
+ end
+ digest
+ end
end
Something went wrong with that request. Please try again.