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

Comments

Move strconv and strings packages' taint-tracking to stdlib, and expand them#318

Closed
gagliardetto wants to merge 5 commits intogithub:mainfrom
gagliardetto:standard-lib-pt-21
Closed

Move strconv and strings packages' taint-tracking to stdlib, and expand them#318
gagliardetto wants to merge 5 commits intogithub:mainfrom
gagliardetto:standard-lib-pt-21

Conversation

@gagliardetto
Copy link
Contributor

@gagliardetto gagliardetto commented Sep 1, 2020 •
edited
Loading

Part of #167

codebox commands:

codebox --out-dir=./generated/latest --pkg=/usr/local/go/src/strconv --http
codebox --out-dir=./generated/latest --pkg=/usr/local/go/src/strings --http

@gagliardetto gagliardetto mentioned this pull request Sep 1, 2020
Closed
@gagliardetto
Copy link
Contributor Author

gagliardetto commented Sep 1, 2020

Should I use a regex for function name matching for similar functions, or it does not make any difference from a performance standpoint?

@smowton
Copy link
Contributor

smowton commented Sep 2, 2020

Personally I like that the particular functions are enumerated rather than using a glob that expresses intent but may hit things it doesn't intend to.

Looks like the second commit, while described as just a move, actually adds support for at least NewReader, causing the test failures seen; suggest changing its commit message to fully describe what it's doing.

Finally I'm surprised to see this doesn't change StdlibTaintFlow.expected -- is that expected?

@gagliardetto gagliardetto force-pushed the standard-lib-pt-21 branch 2 times, most recently from 123034e to afbbe64 Compare September 3, 2020 09:19
@gagliardetto
Copy link
Contributor Author

gagliardetto commented Sep 3, 2020

Finally I'm surprised to see this doesn't change StdlibTaintFlow.expected -- is that expected?

FFR: see #317 (comment)

@gagliardetto gagliardetto changed the title Move strconv and strings packages' taint-tracking to stdlib Move strconv and strings packages' taint-tracking to stdlib, and expand them Sep 4, 2020
max-schaefer
max-schaefer suggested changes Sep 4, 2020
View reviewed changes
Copy link
Contributor

@max-schaefer max-schaefer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for the other PR, LGTM in general, but I'd prefer to drop the taint steps that track individual bytes or runes.

@smowton smowton mentioned this pull request Sep 8, 2020
Merged
@smowton
Copy link
Contributor

smowton commented Sep 11, 2020

@max-schaefer I think the rune comment is applied; I'm going to make a derived PR that introduces the sanitiser for Header.get("authorization").Split(":")[0], otherwise this lgtm

@smowton smowton mentioned this pull request Sep 11, 2020
Merged
@smowton
Copy link
Contributor

smowton commented Sep 11, 2020

Rebased and derived here: #330

@max-schaefer
Copy link
Contributor

max-schaefer commented Sep 14, 2020

Superseded by #330.

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

Reviewers

1 more reviewer

@max-schaefer max-schaefer max-schaefer requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

no-change-note-required

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gagliardetto @smowton @max-schaefer