Conversation
ghost
self-requested a review
as a 
ghost
mentioned this pull request
|
Hello porcupineyhairs 👋 In the meantime, feel free to make changes to the pull request. If you'd like to maximize payout for your this and future submissions, here are a few general guidelines, that we might take into consideration when reviewing a submission.
Please note that these are guidelines, not rules. Since we have a lot of different types of submissions, the guidelines might vary for each submission. Happy hacking! |
yoff
left a comment
yoff
left a comment
There was a problem hiding this comment.
Nice to get this TODO addressed. I do think, though, that it ought to be done slightly differently.
| disablingNode = c and argumentOrigin = c.getArg(1) | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
This class is representing calls to setopt where the first argument is specifically "PyCurl.URL". For these calls, the current implementation of disablesCertificateValidation as none() is correct. What you have done is expressing that this call disables certificate validation if there is another call with arguments (PyCurl.SSL_VERIFYPEER, 0).
What you would need to do is likely to create a new class extending Http::Client::Request::Range representing calls to setopt where the first argument is specifically "PyCurl.SSL_VERIFYPEER" and then you can implement disablesCertificateValidation by comparing the second argument with 0 (and also with False, please).
There was a problem hiding this comment.
Closer, but you are still looking at another call (c = setopt().getACall()). You have to look at the argument for the call represented by the class; something like c = this.getACall(). Also, the boolean has to come from the call; you are now just checking if they wrote false anywhere in the code.
There was a problem hiding this comment.
Yes, much better :-) I think the boolean would also have to go into arg(1)?
| /** Gets a reference to an instance of `pycurl.Curl.SSL_VERIFYPEER`. */ | ||
| private API::Node sslverifypeer() { | ||
| result = API::moduleImport("pycurl").getMember("SSL_VERIFYPEER") | ||
| } |
There was a problem hiding this comment.
This can also come from an instance, see http://pycurl.io/docs/latest/curlobject.html#pycurl.Curl.setopt.
| exists(API::CallNode c | | ||
| c = setopt().getACall() and | ||
| sslverifypeer().getAValueReachableFromSource() = c.getArg(0) and | ||
| exists(IntegerLiteral i | i.getN() = "0" and c.getArg(1).asExpr() = i) |
There was a problem hiding this comment.
It is possible to just compare the integer values
| exists(IntegerLiteral i | i.getN() = "0" and c.getArg(1).asExpr() = i) | |
| exists(IntegerLiteral i | i.getValue() = 0 and c.getArg(1).asExpr() = i) |
|
Hi @porcupineyhairs - we would like to close the last remaining bug bounty PRs soon. Do you think you will keep on working on this PR? |
|
@sylwia-budzynska Sorry for the delay. Last couple of weeks have been busy. I will get this cleared soon. @yoff I have made the changes now. PTAL. However, for some reason, Codeql is giving me duplicate results when I run the original query. |
yoff
left a comment
yoff
left a comment
There was a problem hiding this comment.
Just a small semantic error now, I think :-)
| disablingNode = c and argumentOrigin = c.getArg(1) | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Yes, much better :-) I think the boolean would also have to go into arg(1)?
| exists(IntegerLiteral i | i.getValue() = 0 and this.getArg(1).asExpr() = i) | ||
| or | ||
| exists(BooleanLiteral b | b.booleanValue() = false and this.getArg(_).asExpr() = b) |
There was a problem hiding this comment.
| exists(IntegerLiteral i | i.getValue() = 0 and this.getArg(1).asExpr() = i) | |
| or | |
| exists(BooleanLiteral b | b.booleanValue() = false and this.getArg(_).asExpr() = b) | |
| this.getArg(1).asExpr().(IntegerLiteral).getValue() = 0 | |
| or | |
| this.getArg(1).asExpr().(BooleanLiteral).booleanValue() = false |
I think it is easier to read without the exists, but the important change here is that for the boolean value, we also only look at arg(1).
4 participants
Pycurl is a library which provides curl binding in python. The original library is partially modelled in codeql. This PR adds support to test for SSL certificate validation when using pycurl.