Ruby: add rb/clear-text-storage-sensitive-data query #8306
Conversation
alexrford
force-pushed
the
ruby/clear-text-storage
branch
from
e04a6bf to
7331681
Compare
| private import ruby | ||
| private import codeql.ruby.DataFlow | ||
| private import codeql.ruby.TaintTracking | ||
| import CleartextStorageCustomizations::CleartextStorage |
There was a problem hiding this comment.
I think this import is only required for a use of Source in CleartextStorage.ql - maybe just move this import to that file?
| from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink | ||
| where config.hasFlowPath(source, sink) | ||
| select sink.getNode(), source, sink, "Sensitive data returned by $@ is stored here.", | ||
| source.getNode(), source.getNode().(Source).describe() |
There was a problem hiding this comment.
Looking at some of the results in the DCA evaluation, I wonder if we should report these alerts at the source location rather than the sink location. There seem to be a lot of cases in diaspora/diaspora where a password value flows from a test to a call to User.create or similar. I worry that, since the alert is located at the sink, in models/user.rb, any code-scanning filtering to remove alerts in test files will let it pass through.
There was a problem hiding this comment.
This seems like a good idea overall. The case of a sink in test code and source in production code seems far less likely to occur in practice.
| # BAD: plaintext password stored to disk | ||
| File.new("bar.txt", "a").puts("password: #{new_password}") | ||
| end | ||
| end |
There was a problem hiding this comment.
There's a FP in the evaluation that is this:
random_password = SecureRandom.hex(20)
self.password = random_passwordI don't know if this is due to over-sensitivity in CleartextSources or something else, but I bet this is a common pattern and it would be good to not flag it.
There was a problem hiding this comment.
I guess that the intention in the flagged case here is to set the value to a random hash value where the password that hashes to it is unknown, in which case I agree that it's a FP.
My first approximation to fixing this would is to augment to notSensitiveRegexp() expression to include random as a "safe" term. I have some slight concerns that there could be cases where a random plaintext password is generated and assigned to a field (e.g. for initial signup or password reset) - but it seems unlikely that the field that it's written to would work for both hashed and unhashed passwords. Consequently, in this theoretical case I'd expect there to be the same vulnerability when setting a real password that we could hopefully flag up.
Co-authored-by: Harry Maclean <[email protected]>
No description provided.