GHSA-m8rp-vv92-46c7 has mangled content and formatting since 58f1bbf

EliahKagan commented 1 week ago

Since https://github.com/github/advisory-database/pull/4768 was merged and changes applied in https://github.com/github/advisory-database/commit/58f1bbfff2a4c5fab770f11e3db1c47cb9666422, the global https://github.com/advisories/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...

...on the changes shown in https://github.com/github/advisory-database/pull/4768/files verifies that the actual diff of #4768 does not include these breakages.
...on changes shown in https://github.com/github/advisory-database/commit/58f1bbfff2a4c5fab770f11e3db1c47cb9666422?diff=split&w=0 reveals that the actual diff of https://github.com/github/advisory-database/commit/58f1bbfff2a4c5fab770f11e3db1c47cb9666422 does contain the breakages.

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.

shelbyc commented 3 days ago

Hi @EliahKagan, I tried manually correcting the formatting of the text in the global advisory https://github.com/advisories/GHSA-m8rp-vv92-46c7 according to the repo advisory https://github.com/Byron/gitoxide/security/advisories/GHSA-m8rp-vv92-46c7. Does it look correct to you now?

EliahKagan commented 3 days ago

@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 https://github.com/Byron/gitoxide/security/advisories/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 commented 2 days ago

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!

github / advisory-database