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

Simplify the way to pass the glyph drawing instructions from the worker to the main thread #18015

Merged
merged 1 commit into from
Apr 28, 2024

Conversation

calixteman
Copy link
Contributor

and remove the use of eval in the font loader.

…er to the main thread

and remove the use of eval in the font loader.
@calixteman
Copy link
Contributor Author

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_test from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/c1ca9a99111a8e2/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_test from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/a833d31f5fba71b/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/c1ca9a99111a8e2/output.txt

Total script time: 27.17 mins

  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 18
  different first/second rendering: 3

Image differences available at: http://54.241.84.105:8877/c1ca9a99111a8e2/reftest-analyzer.html#web=eq.log

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/a833d31f5fba71b/output.txt

Total script time: 42.14 mins

  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 2

Image differences available at: http://54.193.163.58:8877/a833d31f5fba71b/reftest-analyzer.html#web=eq.log

@calixteman calixteman merged commit 85e64b5 into mozilla:master Apr 28, 2024
8 of 9 checks passed
@calixteman calixteman deleted the rm_eval_font_loader branch April 28, 2024 21:27
make-github-pseudonymous-again added a commit to infoderm/patients that referenced this pull request May 11, 2024
By default, pdfjs-dist optimizes some path resolution logic by compiling
a JavaScript function on the fly. The function is built using string
concatenation and no effort is made at sanitizing the parts it is
built from. These parts could contain user-input which leads to a code
injection vulnerability. This commit disables this default behavior.
An alternative is to upgrade pdfjs-dist to v4.2.67 or later.

See:
  - https://bugzilla.mozilla.org/show_bug.cgi?id=1893645
  - https://www.cve.org/CVERecord?id=CVE-2024-4367
  - https://security.snyk.io/vuln/SNYK-JS-PDFJSDIST-6810403
  - GHSA-wgrm-67xf-hhpq.
  - mozilla/pdf.js#18015
  - wojtekmaj/react-pdf#1786
  - https://security.stackexchange.com/questions/248462/is-firefoxs-new-javascript-support-within-pdf-files-a-security-concern/248985#248985
  - https://stackoverflow.com/questions/49299000/what-are-the-security-implications-of-the-isevalsupported-option-in-pdf-js
  - mozilla/pdf.js#10818
make-github-pseudonymous-again added a commit to infoderm/patients that referenced this pull request May 11, 2024
By default, pdfjs-dist optimizes some path resolution logic by compiling
a JavaScript function on the fly. The function is built using string
concatenation and no effort is made at sanitizing the parts it is
built from. These parts could contain user-input which leads to a code
injection vulnerability. This commit disables this default behavior.
An alternative is to upgrade pdfjs-dist to v4.2.67 or later.

For reference, see:
  - https://bugzilla.mozilla.org/show_bug.cgi?id=1893645
  - https://www.cve.org/CVERecord?id=CVE-2024-4367
  - https://security.snyk.io/vuln/SNYK-JS-PDFJSDIST-6810403
  - GHSA-wgrm-67xf-hhpq
  - mozilla/pdf.js#18015
  - wojtekmaj/react-pdf#1786
  - https://security.stackexchange.com/questions/248462/\
    is-firefoxs-new-javascript-support-within-pdf-files-a-security-concern/\
    248985
  - https://stackoverflow.com/questions/49299000/\
    what-are-the-security-implications-of-the-isevalsupported-option-in-pdf-js
  - mozilla/pdf.js#10818

Not sure if this will break anything and/or will make certain things
slower.
make-github-pseudonymous-again added a commit to infoderm/patients that referenced this pull request May 11, 2024
By default, pdfjs-dist optimizes some path resolution logic by compiling
a JavaScript function on the fly. The function is built using string
concatenation and no effort is made at sanitizing the parts it is
built from. These parts could contain user-input which leads to a code
injection vulnerability. This commit disables this default behavior.
An alternative is to upgrade pdfjs-dist to v4.2.67 or later.

See:
  - https://bugzilla.mozilla.org/show_bug.cgi?id=1893645
  - https://www.cve.org/CVERecord?id=CVE-2024-4367
  - https://security.snyk.io/vuln/SNYK-JS-PDFJSDIST-6810403
  - GHSA-wgrm-67xf-hhpq.
  - mozilla/pdf.js#18015
  - wojtekmaj/react-pdf#1786
  - https://security.stackexchange.com/questions/248462/is-firefoxs-new-javascript-support-within-pdf-files-a-security-concern/248985#248985
  - https://stackoverflow.com/questions/49299000/what-are-the-security-implications-of-the-isevalsupported-option-in-pdf-js
  - mozilla/pdf.js#10818
github-merge-queue bot pushed a commit to infoderm/patients that referenced this pull request May 11, 2024
By default, pdfjs-dist optimizes some path resolution logic by compiling
a JavaScript function on the fly. The function is built using string
concatenation and no effort is made at sanitizing the parts it is
built from. These parts could contain user-input which leads to a code
injection vulnerability. This commit disables this default behavior.
An alternative is to upgrade pdfjs-dist to v4.2.67 or later.

For reference, see:
  - https://bugzilla.mozilla.org/show_bug.cgi?id=1893645
  - https://www.cve.org/CVERecord?id=CVE-2024-4367
  - https://security.snyk.io/vuln/SNYK-JS-PDFJSDIST-6810403
  - GHSA-wgrm-67xf-hhpq
  - mozilla/pdf.js#18015
  - wojtekmaj/react-pdf#1786
  - https://security.stackexchange.com/questions/248462/\
    is-firefoxs-new-javascript-support-within-pdf-files-a-security-concern/\
    248985
  - https://stackoverflow.com/questions/49299000/\
    what-are-the-security-implications-of-the-isevalsupported-option-in-pdf-js
  - mozilla/pdf.js#10818

Not sure if this will break anything and/or will make certain things
slower.
stephanrauh pushed a commit to stephanrauh/pdf.js that referenced this pull request May 16, 2024
Simplify the way to pass the glyph drawing instructions from the worker to the main thread
@Rob--W
Copy link
Member

Rob--W commented May 22, 2024

This PR is associated with the fix for CVE-2024-4367 .

The advisory currently mentions <= 4.1.392, but in practice the version ranges are different, the precise details are below. Despite the presence of a version range that is unaffected by this specific bug, I still recommend using the latest versions only (v4.2.67+) because these older versions are also affected by another vulnerability (CVE-2018-5158; I've filed a PR to update the right metadata in Github's advisory database at github/advisory-database#4456).

PDF.js versions and statuses:

@zlm15122306538
Copy link

zlm15122306538 commented Jun 12, 2024

and remove the use of eval in the font loader.

If the version is not upgraded, set isEvalSupported: false in which file?

jsBuf.push("c.", current.cmd, "(", args, ");\n");
const commands = [];
for (let i = 0, ii = cmds.length; i < ii; ) {
switch (cmds[i++]) {
Copy link

Choose a reason for hiding this comment

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

This is an odd way to increment a for loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you think we can do better, please file an issue and write a PR, thank you.
Just for your information, each command is followed by its arguments where their length depends on the command.
So here, after i++, i is the position for the first argument, and then each case will increment it depending on the number of arguments.

Copy link

Choose a reason for hiding this comment

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

Apologies -- I didn't realize i was also being incremented in the switch block. I withdraw my comment.

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

Successfully merging this pull request may close these issues.

7 participants