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-m8rp-vv92-46c7 has mangled content and formatting since 58f1bbf #4777

Closed
EliahKagan opened this issue Sep 9, 2024 · 4 comments
Closed

Comments

@EliahKagan
Copy link

Since #4768 was merged and changes applied in 58f1bbf, the global GHSA-m8rp-vv92-46c7 advisory is broken.

Code blocks and code spans have spurious backslashes added to them and, in some cases, literal newlines: literal \ is converted to \\; literal \n sequences converted into a \ followed by a literal newline; literal tabs are converted to single spaces; leading spaces are added in some places where not present; and other changes.

The affected portions of the advisory include its proof-of-concept instructions that, if followed as currently presented, could result in readers assuming they have a patched version when they do not.

This advisory pertains to a vulnerability that is itself about improper handling of backslash-based escape sequences. Consequently, it may be very hard for readers to understand what the advisory is trying to say, even if they are aware of the problem.

There are some problems outside code blocks and code spans, and I am not confident that I have spotted them all, but they include omitted leading spacing for a second paragraph in a numbered list item, causing the list to be broken into two, with the second paragraph outside both lists. This is less of a problem than the changes in preformatted code blocks.

Using this diff tool...

Therefore:

  • This breakage is similar to (though far more extensive than) the breakage observed in connection with an older unrelated advisory GHSA-2mqj-m65w-jghx, where it was manually corrected. However, in #4768 the involvement of the security curation team was not as visible, such that there is not a specific individual I would know to ping.
  • Because the problem is introduced separately from and after a PR s is merged, I do not think I can correctly fix this myself via the improve link or by opening a pull request. Even if there is a change that would restore the old rendering while defensively avoiding mangling, I don't think I have the means to figure out what that change would be, for the reasons described here.

The repo-local advisory remains correct and is usable as a reference.

@EliahKagan EliahKagan changed the title GHSA-m8rp-vv92-46c7 is badly mangled since 58f1bbf GHSA-m8rp-vv92-46c7 has mangled content and formatting since 58f1bbf Sep 9, 2024
@shelbyc
Copy link
Contributor

shelbyc commented Sep 18, 2024

Hi @EliahKagan, I tried manually correcting the formatting of the text in the global advisory GHSA-m8rp-vv92-46c7 according to the repo advisory GHSA-m8rp-vv92-46c7. Does it look correct to you now?

@EliahKagan
Copy link
Author

@shelbyc Thanks!! This is mostly correct now, but there are three remaining problems that I have been able to identify.

Backslashes are doubled in code spans

There are actually no intended occurrences of \\ in the advisory (and \\ does not appear in the advisory at GHSA-m8rp-vv92-46c7), so this can possibly be solved efficiently with search and replace, I am not sure. But in case it is useful, these are the places with \\ that should be \.

I'm presenting them in diff-like form for readability, but these diffs cannot actually be applied as patches because they don't show full lines, and also because they are diffs of Markdown and not of JSON.

-For example, `é` is transformed to `\\303\\251`, with literal backslashes.
+For example, `é` is transformed to `\303\251`, with literal backslashes.
-Its user profile directory is assumed to be `C:\\Users\\Renée`.
+Its user profile directory is assumed to be `C:\Users\Renée`.
-Its user profile directory is assumed to be `C:\\Users\\Ren`.
+Its user profile directory is assumed to be `C:\Users\Ren`.
-Install Git for Windows in the default location for non-systemwide installations, which for that user account is inside `C:\\Users\\Renée\\AppData\\Local\\Programs`.
+Install Git for Windows in the default location for non-systemwide installations, which for that user account is inside `C:\Users\Renée\AppData\Local\Programs`.
-correspondingly modified path components in place of `AppData\\Local\\Programs\\Git`
+correspondingly modified path components in place of `AppData\Local\Programs\Git`
-where attacks such as those shown in the Windows proof-of-concept above can be performed due to the status of `\\` as a directory separator
+where attacks such as those shown in the Windows proof-of-concept above can be performed due to the status of `\` as a directory separator

Subsequent paragraphs in list items are outside the list

In the numbered list under "As Renée:", the first item has a second paragraph, which in the repo advisory is kept as part of the list item by three leading spaces. But in the global advisory, it is a break in the list between the first and second items, rendering unindented.

It should be possible to fix it by re-adding the spaces that had been lost:

-(The scenario can be modified for any location the attacker can predict.
+   (The scenario can be modified for any location the attacker can predict.

Likewise, in that same list, the third item has both the code block and the paragraph after the code block as part of the list, in the repo advisory. But in the global advisory, they are outside the list. This is the same kind of problem as in the first item.

 3. Open a PowerShell window...

-```pwsh
-gix clone ssh://localhost/myrepo.git
-```
+   ```pwsh
+   gix clone ssh://localhost/myrepo.git
+   ```

-At least one, and usually two, instances of the Windows calculator...
+   At least one, and usually two, instances of the Windows calculator...

A spurious double quote mark appears at the end of the "Impact" section

A literal " appears at the very end of the last paragraph.

-because the necessary conditions are uncommon and challenging to predict."
+because the necessary conditions are uncommon and challenging to predict.

It is possible that there are other discrepancies, but I have tried to identify all unintentional differences.

Thanks again for all your help!

@shelbyc
Copy link
Contributor

shelbyc commented Sep 18, 2024

Thank you so, so much for the specific examples that I was able to use when going back and correcting extra \ characters and re-adding removed spaces. If there was a gold star emoji available in the comment reaction menu, that's what I would've given you. Thanks again for reaching out and providing specific examples of discrepancies to fix!

@EliahKagan
Copy link
Author

EliahKagan commented Sep 22, 2024

Thanks! Everything looks great in GHSA-m8rp-vv92-46c7 now, so I'm closing this issue as completed. I will also keep in mind that the specific diffs, even though they cannot be automatically applied as such, are useful for problems like this. In hindsight, I might've been able to produce a full set of those initially.

If it would be helpful, I can open a new issue for the general problem of broken formatting, especially in the presence of backslashes, that this and #3290 (comment) were special cases of. (Other options including not having any public issue for that at this time, and editing and reopening this issue to cover it.)

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

No branches or pull requests

2 participants