Skip to content

Annotations #72

Closed
wants to merge 64 commits into from

4 participants

@daveworth

This is a first pass at ignoring warnings via annotations. Please have a look and tell me what you think!

Dave Worth and others added some commits Apr 4, 2012
Dave Worth - Annotation file creation and loading
- Annotation filtering on demand
- Denotes the number of ignored warnings in reports
977749e
Dave Worth Update README re: Annotations fafdac2
@presidentbeef Add json as a report format de5a9e1
@presidentbeef Use Set[] instead of Set.new([]) 84a719a
@presidentbeef Ignore Model#id for XSS check 882b11e
@presidentbeef Standardize SQL methods to check
because before they were kind of all over the place
c1cf6b6
@presidentbeef Update copyright c208eb0
@presidentbeef Fix check for nested targets in Rails 2 routes 6cbbfc1
@presidentbeef Ignore resource routes if default routes already
fixes bug where resource routes attempted to merge into
:allow_all_actions
a49d60e
@presidentbeef Switch to 1.9.2 on Travis CI
since most development is done with 1.9.3 now
64a8b95
@presidentbeef Add test for select vulnerability in Rails 3 43ba8dd
@presidentbeef Add check for select vulnerability in Rails 3 fab6a9b
@presidentbeef Remove select() as an XSS safe method in Rails 2 e4d661e
@presidentbeef Oops, update expected warnings for Rails 2 tests 8525b2c
@presidentbeef Tests for SafeBuffer vulnerability b58dee6
@presidentbeef Add version check for SafeBuffer vulnerability 087592a
@presidentbeef Add test for skipping CSRF with :except 150a67f
@presidentbeef Add check for skipping CSRF with :except
instead of using :only.
This is essentially a blacklist vs. whitelist issue.
29d98e7
@presidentbeef Bump to 1.5.0
[ci skip]
592b1d3
@presidentbeef README updates 2ddbc6d
@presidentbeef More debug output for current item being processed 39322bd
@presidentbeef Print out stack trace when interrupted (only Ruby 1.9) e3fe89b
@presidentbeef Debug output of which template is being rendered 08440cb
@presidentbeef Cache check result for disabled mass assignment 6cb527c
@presidentbeef Track class and module in BaseProcessor c943903
@presidentbeef Report module in Brakeman::FindCall results
but please don't ever use Brakeman::FindCall if it can
be avoided
24712b1
@presidentbeef Add additional check for global mass assign disable
that looks like this:

module ActiveRecord
  class Base
    attr_accessible
  end
end

Also, could be wrong, but I think old check was broken?
82d0e24
@presidentbeef Fix concatentation of module name cad0c5a
@presidentbeef tracker.config[:rails3] => tracker.options[:rails3] efd00fc
@presidentbeef Support Rails 3 partial render (no :partial => ...)
`render 'blah'` apparently noew renders the partial '_blah'
e194040
@presidentbeef Add test for Rails 3 partial rendoring 70b8b48
@presidentbeef Remove duplicate method on Rails2XSSErubis ce41f07
@presidentbeef Update CHANGES 3bb3331
@presidentbeef Bump to 1.5.1
[ci skip]
8b54267
@presidentbeef Fixes to CheckLinkTo for Rails 2.0 and 2.3
and link_to with a block
d41f480
@presidentbeef Add more tests for link_to 3d997b9
@oreoshake oreoshake I was getting a NoMethodError when rescanning a file in the lib dire…
…ctory.

    NoMethodError: undefined method `process_library' for #<Brakeman::Rescanner:0x10a8f5380>
    /Users/neilm/workspace/brakeman/lib/brakeman/rescanner.rb:77:in `rescan_file'
    /Users/neilm/workspace/brakeman/lib/brakeman/rescanner.rb:50:in `rescan'
    /Users/neilm/workspace/brakeman/lib/brakeman/rescanner.rb:47:in `each'
    /Users/neilm/workspace/brakeman/lib/brakeman/rescanner.rb:47:in `rescan'
    /Users/neilm/workspace/brakeman/lib/brakeman/rescanner.rb:46:in `each'
    /Users/neilm/workspace/brakeman/lib/brakeman/rescanner.rb:46:in `rescan'
    /Users/neilm/workspace/brakeman/lib/brakeman/rescanner.rb:22:in `recheck'
    /Users/neilm/workspace/brakeman/lib/brakeman.rb:291:in `rescan'
    /usr/local/rvm/gems/ree-1.8.7-2011.12/gems/guard-brakeman-0.3.1/lib/guard/brakeman.rb:73:in `run_on_change'
83d2d80
@presidentbeef Output stack trace in debug on interrupt in 1.8.7 ccf4fe9
@presidentbeef Only output stack trace on debug, use caller
Thanks @PragTob
f267043
@presidentbeef Ignore user input in condition of if statements b605bd7
@presidentbeef Don't make file names symbols for --skip-files ad06e39
@presidentbeef Render path check should only warn on user input 0cf8c95
@presidentbeef Medium confidence for user input in render strings f4df29c
@presidentbeef Fix/add dynamic render path tests 70a2824
@presidentbeef Add tests for application using rails_xss 23be7ca
@presidentbeef Fix handling of Erubis templates with xss escaping
either with rails_xss or Rails 3.
This was broken when Brakeman's Erubis output was
changed to match what rails_xss does. Unfortunately, that
broke the ErubisTemplateProcessor such that NO output
was detected. This should fix that.

Note that this code detects auto-escaping by the output variable.
@output_buffer is used in Brakeman's Erubis classes. _buf will
only show up if someone is using Erubis with auto-escaping turned
off.
108b722
@presidentbeef Fix tests after merge 3177af7
@presidentbeef Add utility script for generating tests 27dc2b8
@presidentbeef Oops, need the rails_xss plugin directory for tests fe35bbb
@presidentbeef Use old ruby_parser (2.3.1) for Ruby 1.8 parsing 697be03
@presidentbeef Make sure Sexp patches load for Ruby 1.8 e727cb7
@presidentbeef Fix some Ruby warnings 8cde2ee
@presidentbeef Bump to 1.5.2
[ci skip]
180ca45
@oreoshake oreoshake Warn when user input is supplied to send
Model.send(params[:method]) == bad
95c2046
@oreoshake oreoshake Adding a debug statement 5be94a3
@oreoshake oreoshake Only warn if the target or the first arg contains user input
Add moar test cases
a938139
@presidentbeef Move parsers out of scanner.rb into lib/parsers/ 487221b
@presidentbeef A little more whitespace when showing progress 5a8fc01
@presidentbeef Track module names for controllers
because when the get re-processed the module information
is lost (just class code is stored)
8078779
@presidentbeef Don't double the number of controllers reported
during progress. I don't really know why this was there...
067b9c5
@presidentbeef Add Util.hash_access to simplify accessing hashes 44ca23b
@presidentbeef Handle nested modules in ProcessorHelper 6f65ac7
Dave Worth - Annotation file creation and loading
- Annotation filtering on demand
- Denotes the number of ignored warnings in reports
38e8d27
Dave Worth Merge branch 'annotations' of github.com:daveworth/brakeman into anno…
…tations

Conflicts:
	lib/brakeman/checks/check_send.rb
	test/tests/test_rails2.rb
0774b53
@daveworth daveworth closed this Apr 7, 2012
@daveworth

Sorry, this is way too messy and I didn't notice until after I opened it!

@presidentbeef
Owner

The changes actually looked pretty good!

@daveworth
@cktricky
cktricky commented Apr 8, 2012

Hmm, this looks to be the first steps towards excluding known false positives?

@daveworth

@cktricky Yes it is but this commit was way too ugly after the broken rebasing/merging I did. I've patched it up in Pull request #73. Have a look at that and see what you see. My use of an MD5 to identify a given warning is just good enough I believe but there may be a better approach. Thoughts?

@cktricky
cktricky commented Apr 9, 2012

So, apologies as it is late and I've not looked through the code intensely but each finding is going to have an MD5 hash and you can preclude or ignore those by specifying the hash?

One thing that could be done to make the entropy stronger is using a randomized iInitialization vector, i'm assuming that is what you were going for (randomness)?

@daveworth

@cktricky The idea was to use the MD5 checksum, not for its cryptographic properties, but simply as an unique but deterministic identifier of the hash identifying a given warning. I had originally planned on using Object#hash but it changes with each run of the interpreter. I think using the MD5 sum in the way I am is a bit of a hack but it may be Just Good Enough :TM:

I am definitely open to other ideas.

@cktricky

@daveworth That makes sense, I was just thinking of ways to really randomize the checksum so it would be truly unique so we wouldn't run into any issues with two findings having the same checksum......looking back.....I probably shouldn't write comments when super tired ;-)

FWIW, I like what you did! Def added value.

@presidentbeef presidentbeef locked and limited conversation to collaborators Feb 16, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Something went wrong with that request. Please try again.