Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New CVE YAML field for affected methods? #492

Open
ghbren opened this issue Oct 5, 2021 · 7 comments
Open

New CVE YAML field for affected methods? #492

ghbren opened this issue Oct 5, 2021 · 7 comments

Comments

@ghbren
Copy link

ghbren commented Oct 5, 2021

Would it be possible to enforce a yaml field for the methods affected by each vulnerability?

Almost all CVEs appear to only affect a very small subset of methods, and there is no need to upgrade the bad gems if the affected method are not used. This new field will help us to automatically eliminate the need to upgrade a large set of gems.

@phillmv
Copy link
Member

phillmv commented Oct 6, 2021

I'd be up for adding a field for methods affected, but I don't know that we'd enforce it. I take it you work on Salus at Coinbase & are consuming this data? Are you using affected methods in Salus?

@postmodern
Copy link
Member

This is a really difficult problem as not all vulnerabilities exist in a single method. Often the vulnerable method is not directly called, but called through a chain of other methods, which together lead to the vulnerability. Or the affected method is only vulnerable under certain circumstances or configurations. I feel like describing the affected methods could lead to an imprecise understanding of the vulnerability and ultimately a false sense of security (ex: "we don't call that method directly, so nothing to worry about.").

@ghbren
Copy link
Author

ghbren commented Oct 7, 2021

Yes, I'm at coinbase and work on salus.

Regarding calling a vulnerable method through a chain of other methods. I think we have a solution for that, and please tell me if there are issues with this. Suppose we know that gem A defines the vulnerable methods, and no other methods in gem A call these vulnerable methods. We can then scan the source code for all (transitive) gems (based on Gemfile.lock) that depend on A and see if they call this vulnerable method. If these vulnerable method names do not appear in the transitive gems, then we can conclude there is no need to upgrade gem A.

Re "Or the affected method is only vulnerable under certain circumstances or configurations". Thanks for pointing it out. We are just trying to do something simple so that if we know the vulnerable methods are not called through a chain of other methods, then we can conclude there is no need to upgrade the gem.

@postmodern
Copy link
Member

postmodern commented Oct 7, 2021

Regarding calling a vulnerable method through a chain of other methods. I think we have a solution for that, and please tell me if there are issues with this. Suppose we know that gem A defines the vulnerable methods, and no other methods in gem A call these vulnerable methods. We can then scan the source code for all (transitive) gems that depend on A and see if they call this vulnerable method. If these vulnerable method names do not appear in the transitive gems, then we can conclude there is no need to upgrade gem A.

You are going to run into two major problems in the field of Static Analysis: Graph Theory and the Halting Problem. Analyzing a program to find all possible method calls is a difficult problem, especially in a dynamic language such as Ruby where methods can be monkey-patched in, you can inject or even prepend modules, method_missing, const_missing, and variables do not have fixed types. If you have a vulnerable Method X, spidering out through the call graph to determine if any of the program's code somehow eventually calls down to the vulnerable Method X can become an algorithmically complex problem. Next, it is impossible via Static Analysis alone to determine every possible method call, especially in a dynamic language like Ruby where program state can change as the program is loading/initializing and while it's running. At beast you might be able to detect obvious examples like the scenario you described above, but you will be missing a lot of other edge cases which will result in false negatives.

This seems like a ton of work all to avoid having to run bundle update or incrementing a gem dependency in a Gemfile. It's far less work to simply read the vulnerability advisory to understand the conditions of the vulnerability and it's solutions/mitigations, or just erring on the side of caution and always upgrade the vulnerable gem version.

@ghbren
Copy link
Author

ghbren commented Oct 7, 2021

Thank you very much for your detailed feedback.

I understand how dynamically generated code could cause issues, but my understanding is that the probability would be very small, and techniques like dynamic analysis could reduce such issues. Sometimes upgrading gems breaks other things, so there is a kind of a tradeoff there.

Regarding the graph stuff, given like 10 vulnerable methods, and suppose we have 50 transitive dependency gems, it would not be too time-consuming to "grep" the method names in the 50 gems. In addition, we could cache this info in DB. But we are mostly concerned about newly discovered CVEs. Only a small number of CVEs come out each month, and each CVE has < 10 transitive dependency gems based on our experience, so the probability of running into a halting problem seems extremely small.

Anyway, I totally understand your concern about erring on the side of caution and always upgrade the gem version.

@postmodern
Copy link
Member

postmodern commented Oct 7, 2021

Sometimes upgrading gems breaks other things, so there is a kind of a tradeoff there.

This rarely happens due to the Ruby ecosystem strict adherence to Semantic Versioning. It's almost always possible to upgrade to a patch-level release which only fixes the vulnerability, and does not introduce any non-backwards-compatible changes. There is more risk of your tool giving a false negative for a known vulnerable gem version, and the user (or company) not upgrading the vulnerable gem version, and then the user (or company) eventually getting compromised as a result.

Regarding the graph stuff, given like 10 vulnerable methods, and suppose we have 50 transitive dependency gems

I think you are underestimating the problem. Any method in the program or app could call another method, which in turn could call yet another method, which could call the gem's vulnerable method. Limiting the search only to dependent will give you false negatives, which will impact users.

it would not be too time-consuming to "grep" the method names in the 50 gems

How do you grep for a method call when a gem does obj.send(name,*args)? You then have to figure out the type of obj and the possible values of name and args. This requires things such as SMT Solvers, such as z3, which can represent a variable or a return value of a method as nested algebraic functions which modify some initial input value.

In addition, we could cache this info in DB

And what happens when the data changes between different gem versions? You would probably want to use a Graph Database, such as Neo4j, and track method A -> method B calls noting the version of gem A and gem B. However that makes traversing the call graph extremely painful because you must be aware of every gem version.

Dynamic instrumentation, often used in conjunction with Static Analysis, gives you better coverage, however you still have to deal with the Halting Problem. How do you know your dynamic instrumentation has observed every single code path or edge case? What if a vulnerable code path is executed only when 1) there is data in the database 2) a weekly rake task is ran which sends mass emails out to the user. One does not simply avoid running into the Halting Problem when doing program analysis.

Anyways, this is a hard problem to solve, which is why it hasn't been fully and satisfactorily solved. I wish you the best of luck though. Again, it's easier and more safer to just upgrade from gem 1.2.3 to 1.2.4 using bundler update <gem>.

@ghbren
Copy link
Author

ghbren commented Oct 7, 2021

Thanks again for the detailed reply. You are right, I did underestimate the problem. It was my understanding that everything that could be transitively called would be from gems included in Gemfile.lock. I just realized this is not true, sorry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants