OpenUrlRedirect: Expand safe URL flow configuration #65
Conversation
max-schaefer
left a comment
max-schaefer
left a comment
There was a problem hiding this comment.
Great to hear you were able to reduce the false positives so drastically! I have a few comments. Also, can you please make an issue regarding the two FPs not fixed by this? (No need to tackle them right away.)
Just to clarify: we are still waiting for an evaluation, yes? |
This must have failed, I'll check on it. |
Does that mean that Location and Referer headers can both be considered safe in this context as well? I noticed that a few of the results remaining use those values. |
|
Probably; see my explanation of the typical attack scenario above. |
|
Ok, I'll remove those as sources. |
sauyon
force-pushed
the
openredirect-fps
branch
2 times, most recently
from
4ae9ea4 to
5b9dca4
Compare
|
I've added a few extra things, including something I'd forgotten to change from the LGTM version of the query. I've decided to remove all sources from request headers, because it seems to me that those wouldn't be as attacker-controllable. I've also changed the standard library models for An evaluation (internal link) shows that we now also fix a false positive in our default suite 🎉 |
sauyon
force-pushed
the
openredirect-fps
branch
from
5b9dca4 to
b8c5eef
Compare
max-schaefer
left a comment
max-schaefer
left a comment
There was a problem hiding this comment.
LGTM, modulo one small comment.
Also, since this query was already in 1.23 we should add a change note for 1.24.
|
Ok, added a change note. |
max-schaefer
left a comment
max-schaefer
left a comment
There was a problem hiding this comment.
One minor nit: we usually quote the query @name, not the @description, in the change notes.
change-notes/1.24/analysis-go.md
Outdated
| | Reflected cross-site scripting (`go/reflected-xss`) | Fewer results | Untrusted input flowing into an HTTP header definition or into an `fmt.Fprintf` call with a constant prefix is no longer flagged, since it is in both cases often harmless. | | ||
| | Useless assignment to field (`go/useless-assignment-to-field`) | Fewer false positives | The query now conservatively handles fields promoted through embedded pointer types. | | ||
| | Bitwise exclusive-or used like exponentiation (`go/mistyped-exponentiation`) | Fewer false positives | The query now identifies when the value of an xor is assigned to a mask object, and excludes such results. | | ||
| | Open URL redirection based on unvalidated user input may cause redirection to malicious websiets (`go/unvalidated-url-redirection`) | Fewer false positives | The query now identifies some sources that are not attacker-controlled, and excludes results with such sources. | |
There was a problem hiding this comment.
| | Open URL redirection based on unvalidated user input may cause redirection to malicious websiets (`go/unvalidated-url-redirection`) | Fewer false positives | The query now identifies some sources that are not attacker-controlled, and excludes results with such sources. | | |
| | Open URL redirect (`go/unvalidated-url-redirection`) | Fewer false positives | The query now identifies some sources that are not attacker-controlled, and excludes results with such sources. | |
Also add some more tests
sauyon
force-pushed
the
openredirect-fps
branch
from
69c5e63 to
fbc2499
Compare
2 participants
Tested this on LGTM.com and it fixes all but two of the obvious false positives.
The last two are these results on
caddyserver/caddy, for which the source, which I'd expect to be on line 102 offileserver.go, is strangely not found. I'll probably do some digging into this eventually, but I thought I'd leave it here for now since it seems like this may be a bit more involved.