Skip to content

Conversation

kaeluka
Copy link

@kaeluka kaeluka commented Mar 14, 2022

This adds a taint step that has been missing in the handlebars templating library. Passing data to a template may flow to custom 'helpers'. See the attached test and predicate documentation for details :)

Related CVE:

CVE-2022-24718

DCA

Experiment looking good.

@github-actions github-actions bot added the JS label Mar 14, 2022
@kaeluka kaeluka changed the title Js/CVE 2022 24718 Add taint step for handlebars model Mar 14, 2022
@owen-mc owen-mc changed the title Add taint step for handlebars model JS: Add taint step for handlebars model Mar 15, 2022
@kaeluka kaeluka marked this pull request as ready for review March 24, 2022 11:56
@kaeluka kaeluka requested a review from a team as a code owner March 24, 2022 11:56
@kaeluka kaeluka added the no-change-note-required This PR does not need a change note label Mar 24, 2022
Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

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

Generally looks OK, but I got some comments.

@erik-krogh
Copy link
Contributor

The autoformatter is failing on Handlebars.qll.

Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

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

I did an evaluation.
(nightly-old + a bunch of random projects that use handlebars.compile(..).

I'm a bit worried about false flow in the case where we don't know the template string, but otherwise LGTM.

@kaeluka
Copy link
Author

kaeluka commented Apr 14, 2022

@erik-krogh I think I've addressed all concerns.

Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

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

👍

@kaeluka kaeluka merged commit 2fb3147 into github:main Apr 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JS no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants