Skip to content

C++/Docs: add example based on NtohlArrayNoBound #2318

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

Conversation

rdmarsh2
Copy link
Contributor

This adds a simple example of using the taint tracking library, including overriding the isSanitizer predicate, to the data flow library documentation.

@rdmarsh2 rdmarsh2 requested review from jf205 and a team November 13, 2019 21:24
@rdmarsh2 rdmarsh2 added this to the 1.23 milestone Nov 13, 2019
geoffw0
geoffw0 previously approved these changes Nov 14, 2019
Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

Query LGTM. I'm not entirely familiar with the context.

jbj
jbj previously approved these changes Nov 14, 2019
@jbj
Copy link
Contributor

jbj commented Nov 14, 2019

Docs team, feel free to merge when you're happy.

@@ -244,6 +244,49 @@ The following data flow configuration tracks data flow from environment variable
select fopen, "This 'fopen' uses data from $@.",
getenv, "call to 'getenv'"

The following taint tracking configuration tracks data from a call to ``ntohl`` to an array index operation. It uses the ``Guards`` library to recognize expressions that have been bounds checked and avoid propagating taint through them. It also uses ``isAdditionalTaintStep`` to add flow from loop bounds to loop indexes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The following taint tracking configuration tracks data from a call to ``ntohl`` to an array index operation. It uses the ``Guards`` library to recognize expressions that have been bounds checked and avoid propagating taint through them. It also uses ``isAdditionalTaintStep`` to add flow from loop bounds to loop indexes.
The following taint-tracking configuration tracks data from a call to ``ntohl`` to an array index operation. It uses the ``Guards`` library to recognize expressions that have been bounds checked and avoid propagating taint through them. It also uses ``isAdditionalTaintStep`` to add flow from loop bounds to loop indexes.

Hyphenate 'taint-tracking' when it's an adjective.

@@ -244,6 +244,49 @@ The following data flow configuration tracks data flow from environment variable
select fopen, "This 'fopen' uses data from $@.",
getenv, "call to 'getenv'"

The following taint tracking configuration tracks data from a call to ``ntohl`` to an array index operation. It uses the ``Guards`` library to recognize expressions that have been bounds checked and avoid propagating taint through them. It also uses ``isAdditionalTaintStep`` to add flow from loop bounds to loop indexes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Second sentence doesn't quite flow for me. Does this make sense?

It uses the Guards library to recognize expressions that have been bounds checked, and therefore don't propagate taint.

Copy link
Contributor Author

@rdmarsh2 rdmarsh2 Nov 14, 2019

Choose a reason for hiding this comment

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

That reads differently to me, since it implies that taint not propagating through bounds-checked expressions when using this confiugration is a property of the expression rather than a property of the taint-tracking configuration. Would this be better:

It uses the Guards library to recognize expressions that have been bounds-checked and defines isSanitizer to prevent taint from propagating through them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that's better. I misunderstood it before, but it makes more sense now. Thanks.

@rdmarsh2 rdmarsh2 dismissed stale reviews from jbj and geoffw0 via dad1c96 November 14, 2019 22:54
@joshhale joshhale changed the base branch from master to rc/1.23 November 15, 2019 15:00
@jbj
Copy link
Contributor

jbj commented Nov 25, 2019

@jf205 Please merge if you're happy with the changes.

Copy link
Contributor

@shati-patel shati-patel left a comment

Choose a reason for hiding this comment

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

James is on holiday today. You've addressed all his comments, so I'll merge.

@shati-patel shati-patel merged commit 9b5437c into github:rc/1.23 Nov 25, 2019
@jf205
Copy link
Contributor

jf205 commented Nov 26, 2019

Thanks for the merge @shati-patel.
Apologies @rdmarsh2 for not noticing the updates myself.

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

Successfully merging this pull request may close these issues.

5 participants