Skip to content
This repository was archived by the owner on Jan 5, 2023. It is now read-only.

Add taint-tracking for packages inside net/* (except for net/url, which was left as-is) #337

Merged
merged 9 commits into from
Sep 21, 2020

Conversation

gagliardetto
Copy link
Contributor

@gagliardetto gagliardetto commented Sep 12, 2020

codebox commands:

codebox --out-dir=./generated/latest --pkg=/usr/local/go/src/net --http
codebox --out-dir=./generated/latest --pkg=/usr/local/go/src/net/http --http
codebox --out-dir=./generated/latest --pkg=/usr/local/go/src/net/http/httputil --http
codebox --out-dir=./generated/latest --pkg=/usr/local/go/src/net/mail --http
codebox --out-dir=./generated/latest --pkg=/usr/local/go/src/net/textproto --http

Part of #167

@gagliardetto gagliardetto changed the title @gagliardetto Add taint-tracking for package net Add taint-tracking for packages inside net/* (except for net/url, which was left as-is) Sep 12, 2020
@smowton
Copy link
Contributor

smowton commented Sep 14, 2020

Will run another batch of comparsions for #331 #332 #333 #334 #335 #336 #337

@gagliardetto
Copy link
Contributor Author

That's exciting news! Thanks!

@smowton
Copy link
Contributor

smowton commented Sep 14, 2020

Partial results:

New true positives (I think): 5
New false positives: 4

The only FP attributable directly to this PR is regarding the results of ParseIP as dangerous. It's clearly dangerous to parse a user IP and then open a socket to it, for example, but it's not dangerous in most contexts (XSS, SQLi and so on), so I recommend functions like that which tightly constrain the possible form of the input string should not be modelled by default. A query looking for particular circumstances where a user-controlled IP or MAC address or similar is dangerous can re-include them.

@smowton
Copy link
Contributor

smowton commented Sep 15, 2020

Performance comparison results are unremarkable. New hits from that run:

1x new allocation-size-overflow -- a marginal case (we're complaining about len(usersupplied) + 8 possibly overflowing), but this PR's contribution is in propagating taint across a regexreplaceall, which is correct.

1x new clear-text logging -- a false positive due to a bearer token that the service parses into a non-secret and a secret component, logging the non-secret. Probably too app-specific to fix.

1x new path injection -- allows an attacker to explore the FS by stat'ing any file they wish, without apparent escaping. Could be an FP if the server uses an OS-level mechanism to limit file accesses.

1x new request-forgery -- an open HTTP proxy

@gagliardetto
Copy link
Contributor Author

Thanks @smowton and @max-schaefer for all your effort and help in bringing this project to life!

I'll go ahead and submit the last 7 new PRs that will complete the standard library taint-tracking. Or I could wait for next week, but I have time today.

@smowton
Copy link
Contributor

smowton commented Sep 16, 2020

Once this is resolved and the os package PR is passing tests, then suggest making a big PR that combines all this second tranche (everything up to #337), so we can merge them en masse without repeatedly resolving conflicts on Stdlib.qll.

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

Successfully merging this pull request may close these issues.

2 participants