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

[GHSA-7jg2-jgv3-fmr4] The PDF viewer does not sufficiently sanitize PostScript... #4456

Merged

Conversation

Rob--W
Copy link

@Rob--W Rob--W commented May 21, 2024

Updates

  • Affected products
  • Description
  • References
  • Source code location
  • Summary

Comments
Added affected versions, projects, commit and PR. The PR is already referenced by the bugzilla.mozilla.org link associated with the CVE.

@darakian
Copy link
Contributor

Hey @Rob--W, many thanks for the PR but a couple questions for you

  1. I'm not sure I follow on the linkage between the PR and commit you've added in this PR and this specific issue. Any chance you can elaborate on how it's linked? The commit seems to be talking about some some intermediate representation (I think) in the pdf library, but doesn't seem to call out any sort of injection.
  2. On your versions; if the fixed version is 2.0.943 would it make sense to simply drop the last_known_affected_version_range": "<= 1.10.100? There looks to be one version between those two (2.0.550). See: https://www.npmjs.com/package/pdfjs-dist?activeTab=versions

@Rob--W
Copy link
Author

Rob--W commented May 23, 2024

Hey @Rob--W, many thanks for the PR but a couple questions for you

  1. I'm not sure I follow on the linkage between the PR and commit you've added in this PR and this specific issue. Any chance you can elaborate on how it's linked? The commit seems to be talking about some some intermediate representation (I think) in the pdf library, but doesn't seem to call out any sort of injection.

The canonical location for the bug report is at https://bugzilla.mozilla.org/show_bug.cgi?id=1452075, which was already linked in the advisory before my proposed advisory update. That ticket links to the PR that fixes the issue, which is at mozilla/pdf.js#9659. When that PR was merged, the commit was mozilla/pdf.js@2dc4af5.

  1. On your versions; if the fixed version is 2.0.943 would it make sense to simply drop the last_known_affected_version_range": "<= 1.10.100? There looks to be one version between those two (2.0.550). See: https://www.npmjs.com/package/pdfjs-dist?activeTab=versions

I used the releases in the source repository as a reference of the release, which didn't list versions between v1.10.100 and v2.0.943. I'll double-check the versions and update this PR.

P.S. I filed a web form which auto-generated this PR. How should I update this PR?

@darakian
Copy link
Contributor

darakian commented May 23, 2024

The canonical location for the bug report is at https://bugzilla.mozilla.org/show_bug.cgi?id=1452075, which was already linked in the advisory before my proposed advisory update. That ticket links to the PR that fixes the issue, which is at mozilla/pdf.js#9659. When that PR was merged, the commit was mozilla/pdf.js@2dc4af5.

Gotcha, thanks!

P.S. I filed a web form which auto-generated this PR. How should I update this PR?

You can make commits or I can make the edits on my end if its easier for you to just leave a comment here 👍

@Rob--W
Copy link
Author

Rob--W commented May 26, 2024

I took a closer look to find the exact version ranges:

In short, the exact versions are:

  • vulnerable: <= 2.0.489 (except 1.10.100)
  • fixed: >= 2.0.550 and 1.10.100

@darakian I don't know the convention that you use to express non-linear fixes, so please use my provided information to update my PR before merging.

@advisory-database advisory-database bot merged commit 665a2b4 into Rob--W/advisory-improvement-4456 May 28, 2024
2 checks passed
@advisory-database advisory-database bot deleted the Rob--W-GHSA-7jg2-jgv3-fmr4 branch May 28, 2024 20:43
@advisory-database
Copy link
Contributor

Hi @Rob--W! Thank you so much for contributing to the GitHub Advisory Database. This database is free, open, and accessible to all, and it's people like you who make it great. Thanks for choosing to help others. We hope you send in more contributions in the future!

@darakian
Copy link
Contributor

Many thanks for digging. Double checking on npmjs.org it looks to me like 2.0.550 does have the updated code based on the changes in that commit 👍
https://www.npmjs.com/package/pdfjs-dist/v/2.0.550?activeTab=code

@darakian I don't know the convention that you use to express non-linear fixes, so please use my provided information to update my PR before merging.

As two ranges :)
I've gone ahead and split the range into two ( < 1.10.100 && >= 2.0.0, < 2.0.550). Many thanks for the PR! 👍

@Rob--W
Copy link
Author

Rob--W commented May 28, 2024

I don't see this entry listed at https://github.com/mozilla/pdf.js/security

What is necessary to get it there?

@darakian
Copy link
Contributor

darakian commented May 28, 2024

What is necessary to get it there?

That would be under the control of mozilla. You'll need to get the repo owner to update that entry. The way our system works is that we have repo advisories and global advisories where we (github) review and maintain the global ones
ex.
GHSA-7jg2-jgv3-fmr4
and the repo advisories remain under the control of the repo owner. Not the cleanest system, but it's the one we have.

@Rob--W
Copy link
Author

Rob--W commented May 29, 2024

There doesn't seem to be a way for repo admins to link the existing one, at least not from the web interface.

Next to the repo-specific advisory creation page, there is this note:

Once published, security advisories on public repositories are visible to everyone.

Once reviewed by GitHub, security advisories may be broadcast on the GitHub Advisory Database. They may also trigger Dependabot alerts to users that depend on this repository.

This suggests that there is an automatic flow to publish repo-specific advisories in this global advisory-database. A nice characteristic of this feature is that the slugs are the same, for example:

How about the other way around? In this case there is already an old existing advisory, and I'd like to have it to be available in the PDF.js repo (preferably with the same metadata and timestamps).

It looks like the usual workflow is for a repo-specific security advisory to be published first, followed by a global advisory.

The documentation on Publishing a repository security advisory states "Edits to global advisories will not change or affect how the advisory appears on the repository.", which makes it clear that there is no automatic synchronization from the global to the repo-specific advisories. But what if this is exactly what I want, whether automatically or manually?

@darakian
Copy link
Contributor

There doesn't seem to be a way for repo admins to link the existing one, at least not from the web interface.

Not sure I follow here. The repo admin(s) should be able to update their repo advisory at any time.

This suggests that there is an automatic flow to publish repo-specific advisories in this global advisory-database. A nice characteristic of this feature is that the slugs are the same, for example:

It's semi-automatic. We have humans (like myself) in that loop for validation and enrichment and the global advisory is what powers dependabot (hence humans).

How about the other way around? In this case there is already an old existing advisory, and I'd like to have it to be available in the PDF.js repo (preferably with the same metadata and timestamps).

There is zero automatic flow the other way around. The repo and global versions of the advisories are essentially independent.

But what if this is exactly what I want, whether automatically or manually?

That would be a feature request which I can forward along for you 😄

@Rob--W
Copy link
Author

Rob--W commented May 29, 2024

There doesn't seem to be a way for repo admins to link the existing one, at least not from the web interface.

Not sure I follow here. The repo admin(s) should be able to update their repo advisory at any time.

Your statement is true, but the question is not whether a new repo-specific advisory can be created, but whether the global advisory can be copied to the repo-specific one.

To avoid confusion, I want the same GHSA identifier for both, and ideally the same old timestamp (and other metadata) so that it is obvious that it is the same older security issue. Is this possible?

How about the other way around? In this case there is already an old existing advisory, and I'd like to have it to be available in the PDF.js repo (preferably with the same metadata and timestamps).

There is zero automatic flow the other way around. The repo and global versions of the advisories are essentially independent.

I thought that there was some automation because the GHSA identifier for the two examples above are the same, which suggests that there may be some automatic link between the global and repo-specific advisory.

But what if this is exactly what I want, whether automatically or manually?

That would be a feature request which I can forward along for you 😄

Thanks! It's not clear what the specific request would be, but I'd welcome any feature that enables me to achieve my goal.

@darakian
Copy link
Contributor

Your statement is true, but the question is not whether a new repo-specific advisory can be created, but whether the global advisory can be copied to the repo-specific one.

It would need to be manually updated on the repo side to mirror the content on the global advisory.

To avoid confusion, I want the same GHSA identifier for both, and ideally the same old timestamp (and other metadata) so that it is obvious that it is the same older security issue. Is this possible?

I don't think so. I don't 100% know, but I doubt metadata could be made to be synchronous without some tooling changes on our end.

I thought that there was some automation because the GHSA identifier for the two examples above are the same, which suggests that there may be some automatic link between the global and repo-specific advisory.

There is automation in that a repo advisory creates the global advisory for us to review. By policy we try to respect the intent of the repository author, but we also have stricter guidelines for content that will end up in front of all of github than any given repo owner may have. We're sort of an editorial pass on top of what repo owners publish where we make sure things are valid, of general interest, de-jargon'd (as much as we can), and so forth for a general audience.

Thanks! It's not clear what the specific request would be, but I'd welcome any feature that enables me to achieve my goal.

I can make a pretty general issue perhaps as opt-in behavior for a repo owner to accept our edits back to their repo advisory. Would you mind if add the email on your github profile to the issue as a contact as well?

@Rob--W
Copy link
Author

Rob--W commented May 29, 2024

To avoid confusion, I want the same GHSA identifier for both, and ideally the same old timestamp (and other metadata) so that it is obvious that it is the same older security issue. Is this possible?

I don't think so. I don't 100% know, but I doubt metadata could be made to be synchronous without some tooling changes on our end.

I don't mind some manual one-time curation. My main wishes are:

  1. have the same GHSA ID for the same issue
  2. have the same publication date

Thanks! It's not clear what the specific request would be, but I'd welcome any feature that enables me to achieve my goal.

I can make a pretty general issue perhaps as opt-in behavior for a repo owner to accept our edits back to their repo advisory. Would you mind if add the email on your github profile to the issue as a contact as well?

I don't mind, my email is not a secret.

@darakian
Copy link
Contributor

Right on. I've got an issue made. I can't promise any sort of timelines of course, but this is now at least visible for future planning :)

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.

2 participants