Skip to content

Taint propagation for StringBuffer conversion#291

Merged
tmbrbr merged 4 commits into
SAP:mainfrom
leeN:Substring_taint_fixes
May 23, 2025
Merged

Taint propagation for StringBuffer conversion#291
tmbrbr merged 4 commits into
SAP:mainfrom
leeN:Substring_taint_fixes

Conversation

@leeN

@leeN leeN commented May 20, 2025

Copy link
Copy Markdown
Collaborator

No description provided.

@leeN leeN requested a review from tmbrbr May 20, 2025 20:17
@leeN leeN self-assigned this May 20, 2025
@leeN leeN mentioned this pull request May 20, 2025
Closed
tmbrbr
tmbrbr reviewed May 21, 2025
View reviewed changes

@tmbrbr tmbrbr left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure that this is the right approach, as we overwrite the taint of the underlying StringBuffer (which may have different taint to the String on top). My preference would be to make sure that taint info is propagated after GetStringBuffer is called.

In other words reimplement the logic here (from the v128.0 tag):

project-foxhound/js/xpconnect/src/XPCString.cpp

Lines 111 to 123 in e0033ca

if (StringBuffer* buf = readable.GetStringBuffer()) {
bool shared;
if (!UCStringBufferToJSVal(cx, buf, length, vp, &shared)) {
return false;
}
if (shared) {
*sharedBuffer = buf;
}
if (readable.isTainted()) {
JS_SetTaint(cx, vp, readable.Taint());
}
return true;
}

This got lost during the merge unfortunately. What do you think?

@leeN

leeN commented May 21, 2025

Copy link
Copy Markdown
Collaborator Author

Hmmm, I see. So, effectively patch up all call sites of GetStringBuffer()? I can give that a try during some meetings today, sure.

@tmbrbr

tmbrbr commented May 21, 2025

Copy link
Copy Markdown
Contributor

Hmmm, I see. So, effectively patch up all call sites of GetStringBuffer()? I can give that a try during some meetings today, sure.

Exactly, that would be my preference. Would be great if you can look into it!

@leeN

leeN commented May 21, 2025

Copy link
Copy Markdown
Collaborator Author

I assume you meant like this? :)

@leeN

leeN commented May 21, 2025

Copy link
Copy Markdown
Collaborator Author

So, sorry, I understood your concern now and have resolved it. Please squash these commits when merging :)

tmbrbr
tmbrbr approved these changes May 23, 2025
View reviewed changes

@tmbrbr tmbrbr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good! Thanks for the changes!

@tmbrbr tmbrbr merged commit 5d114fd into SAP:main May 23, 2025
12 checks passed
@tmbrbr tmbrbr mentioned this pull request May 26, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tmbrbr tmbrbr tmbrbr approved these changes

Assignees

@leeN leeN

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@leeN @tmbrbr