Skip to content
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

Js math taint propagation #3

Closed
wants to merge 15 commits into from

Conversation

alexbara2000
Copy link

Added taint propagation for the JSMath builtin library.
Added tests for the JSMath builtin library propagation.

@alexbara2000 alexbara2000 changed the base branch from main to primitaint-merge August 1, 2024 21:21
@leeN
Copy link

leeN commented Aug 2, 2024

First of all: Thanks for contributing to foxhound!

I had a cursory look at the changes, and they look fine. What's slightly unclear to me is the intention behind these changes. Could you provide some examples of JavaScript code that does not propagate taints if one uses the primitaint-merge HEAD vs. your changes?
That would be fantastic :)
You probably spoke with Tom about this during a call, but for reference, it might be useful to spell this out here again.

@alexbara2000
Copy link
Author

Yes of course, I will provide a bit more information about the changes. Javascript has a builtin library called Math which gives users access to math functions like Math.round(x). Since this library is builtin, it needs to be defined in the JS engine which in the case of Firefox it can be found in js/src/jsmath.cpp.

The issue was that this builtin library was not taint aware meaning that any JS code that uses it would loose it's taint. The simplest example is if you have the following statement var a = Math.round(b). Before these changes, if b was tainted, a would not be tainted. The tests that I created are examples of operations that would loose their taint.

This pull request solves this issue by making the whole math library taint aware. Math library is where you can find all the methods that are part of the math library which I have made taint aware. I can also give a more low level explanation of the changes I made if needed.

@leeN
Copy link

leeN commented Aug 2, 2024

Ah, interesting, thank you! That makes a ton of sense :)

I wonder how this might have impacted the fp-tracer study, as max/min/round seem fairly common operations heh.

js/src/jsmath.cpp Outdated Show resolved Hide resolved
NumberObject *taintedResult = NumberObject::create(cx, 0);
bool isTainted = false;

if(isTaintedNumber(lhs)){
Copy link
Owner

Choose a reason for hiding this comment

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

Another thing I noticed - I think a lot of this code can be replaced by a call to JS::getAnyNumberTaint in jstaint.cpp.

Have a look at e.g. the AddOperation in Interpreter-inl.h for an example how this is done.

I have recently (last week!) updated the logic for combining two taint flows together so that in principle both parents in the taint flow are saved. If you use the getAnyNumberTaint call then all of this will be taken in account automatically.

Copy link
Owner

Choose a reason for hiding this comment

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

PS - you might need to update your branch to the HEAD of primitaint-merge to get the latest changes.

leeN and others added 5 commits August 5, 2024 12:15
In this branch properties of WebIDL files can be marked as taint
sources with an attribute. This is super nice, but if one wants to
disable one of them one has to note down their names during a build.
This is pretty cumbersome. This commit tries to resolve this issue,
as it simply adds every IDL based source to the properties
defaults.
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.

4 participants