Follow-ups on CVE-2024-35186 and CVE-2024-35197

[EliahKagan]

Following up on the vulnerabilities fixed in May and their security advisories – CVE-2024-35186 (GHSA-7w47-3wg8-547c) and CVE-2024-35197 (GHSA-49jc-r788-3fc9) – it looks like there are a few administrative things that should still be done.

RUSTSEC

The main thing is that there are still no RUSTSEC advisories for either of the two vulnerabilities.

We expected the advisories might be imported automatically from the GitHub Advisory Database into RUSTSEC. It seems from rustsec/rustsec#656 that such things might be possible. But this does not seem to be set up to happen automatically.

So their RUSTSEC advisories should still be created. I plan to open PRs on the advisory-db repository to achieve this. If and when I do, I'll add links here to them. There are some unresolved questions, detailed as follows, that complicate, and may temporarily block, my PRs for that.

The only information that goes into RUSTSEC advisories that I'm unsure about for them--though from a RUSTSEC perspective it seems to be very important information--is which of the affected crates each of the vulnerabilities should be listed as affecting. As I understand it, a RUSTSEC advisory always corresponds to exactly one crate.

If necessary, multiple RUSTSEC advisories could be created for the same vulnerability. For crates that are affected for dependency-related reasons, some can be omitted. So separate RUSTSEC advisories shouldn't be needed for all the crates listed in the repository and GitHub Advisory Database advisories:

• For CVE-2024-35186 (GHSA-7w47-3wg8-547c), I think it might even be possible to identify a single crate as the only one that needs a RUSTSEC advisory, though I am not sure. (Originally I had thought of this as primarily affecting gix-worktree-state, but based on where most of the code changes happened, I am no longer at all certain of that.)
• For CVE-2024-35197 (GHSA-49jc-r788-3fc9), this seems much less likely, because fetching refs that matched reserved names and checking out files that matched reserved names were done by different code in different crates. It looks to me that the advisory there will have to become two (or more, but probably just two) separate RUSTSEC advisories, though they could have the same text.

Hyperlinks in the May 2024 update post

In #1412, in the "Safety First" section, hyperlinks with the titles of the two vulnerabilities are given. However, these inadvertently have the same URL: both are GHSA-7w47-3wg8-547c.

The second link, for the "Refs and paths" vulnerability, should be changed to GHSA-49jc-r788-3fc9.

The pull request description

This doesn't have to be done, and whether it ought to be done is subjective. But it seems to me that the original checklist of items related to fixing these two vulnerabilities should be added to the description of #1374, which currently lists only the other items, not related to the security fixes that I think were the main impact of that PR. None of the information there is in any way sensitive any longer.

The checklist I'm talking about corresponds to 79dce79, which merged a pull request from a private fork but came in as part of #1374. My rationale for including it is that it would make it easier for people in the future to figure out what changes that PR brought in and why, and also that it would help direct people who want to see how these vulnerabilities were fixed to a high-level overview of the changes that fixed them.

These could be posted as a comment if desired, and I can post them as such. But I figured, since I was opening this question post anyway, that I might as well inquire first, in case you do want them in the description itself (I cannot edit that), or in case there is some reason you think it wouldn't be helpful.

[Byron]

Thanks for bringing this up!

The main thing is that there are still no RUSTSEC advisories for either of the two vulnerabilities.

Would @Shnatsel know how this usually works?

In #1412, in the "Safety First" section, hyperlinks with the titles of the two vulnerabilities are given. However, these inadvertently have the same URL: both are GHSA-7w47-3wg8-547c.

The second link, for the "Refs and paths" vulnerability, should be changed to GHSA-49jc-r788-3fc9.

Thanks you! It's fixed now.

This doesn't have to be done, and whether it ought to be done is subjective. But it seems to me that the original checklist of items related to fixing these two vulnerabilities should be added to the description of #1374, which currently lists only the other items, not related to the security fixes that I think were the main impact of that PR. None of the information there is in any way sensitive any longer.

Thank you, a good call, both to secure this information, and for making it public. It's done now.

[Shnatsel]

I've just been busy and didn't import them when they appeared, and it slipped my mind afterwards.

We really should finish up and deploy import from GHSA. But right now a PR against https://github.com/rustsec/advisory-db/ is the way to expedite things.

[EliahKagan]

Thanks! I've opened rustsec/advisory-db#1996 and rustsec/advisory-db#1997. I am not sure if they're ready to go, however, mainly relating to the question of which and how many crates need notices. I believe it is more than one in each case, but significantly fewer than the number of crates listed as affected in the repository-local and GitHub Advisory Database global advisories. I've detailed that and other potential areas of improvement in the PR descriptions.

@Byron You may want to take a look at those PRs, in case you notice any mistakes or have a preference as to how things should be done. I'm particularly interested in a double-check on whether it's okay that I have omitted gix-worktree-state as requiring a notice. This is the same as in other crates that are affected mainly because they consume affected crates, but in the case of gix-worktree-state the behavior really is sort of conceptually centered there, so my belief that it doesn't need a notice deserves some scrutiny. Also, its code did need to be changed, but I think it may be that dependency resolution would prevent an old unfixed version of it from being used with the new version of gix-worktree.

In addition, and mostly separate from the forthcoming RUSTSEC advisories and the advisory-db PRs, while working on this I noticed that it looks like my listing of gix-fs as affected by the Windows device names vulnerability CVE-2024-35197 (GHSA-49jc-r788-3fc9) seems to have been mistaken. The key security-related change to gix-fs in #1374, if I understand correctly, was:

https://github.com/Byron/gitoxide/blob/e2e8cf9206058365be50ea899cc4dcc22e923bef/gix-fs/src/stack.rs#L103-L111

While this is an extremely important part of the fix for the directory traversal vulnerability CVE-2024-35186 (GHSA-7w47-3wg8-547c), it is separate from the case of path components that clash with reserved legacy DOS-style Windows device names like CON and COM1. I think both vulnerabilities' fixes included important improvements in gix-index and gix-worktree, but that the changes in gix-fs were only part of the fix for the directory traversal vulnerability (and of course the changes in gix-ref were only a part of the fix for the reserved names vulnerability).

Assuming that analysis is correct, I was wrong to have listed gix-fs for CVE-2024-35197 (GHSA-49jc-r788-3fc9) and should remove it from the list of affected crates in the repo-local advisory GHSA-49jc-r788-3fc9 and submit a PR to do the same in the CVE-2024-35197 global advisory.

Mainly in case there are other changes that should also be made to those advisories, I'm holding off on making those changes for a short while. But you can also let me know if you think I was right about gix-fs originally and mistaken now.

[Byron]

Thanks for submitting the PRs and for sharing your insights!

Regarding gix-worktree-state, it feels like since it deals with checkout, it should be part of the advisory like it was initially. However, it may be that such a fix is automatically inherited by fixed upstream crates in which case it probably doesn't have to be listed. The confusion arises with you mentioning that its code had to be changed, so it seems necessary to mention it after all.

But I am also sure I am missing a crucial point, and if in doubt, I'd say you are making the right call.

Regarding gix-fs, I think the analysis is correct as it really is only involved in preventing relative path components.

[EliahKagan]

Regarding gix-fs, I think the analysis is correct as it really is only involved in preventing relative path components.

I'll remove that in the repository-local advisory and open a PR on the GitHub Advisory Database to do the corresponding removal in the global advisory.

I may wait a short while to do so, in case any other accompanying changes should be made to those GHSA advisories that will be discovered during the process of figuring out what to do for the RUSTSEC advisories (though my guess is that there will most likely not be other needed changes in the GHSA advisories).

The confusion arises with you mentioning that its code had to be changed, so it seems necessary to mention it after all.

I think some code had to be changed in each of the crates listed in the GHSA advisories (except that the gix-fs code only had to be changed to fix one vulnerability, and not the other). There were other crates that changed in #1374 but where the change was not fixing the vulnerability in that crate. In particular, gix-validate had huge changes, with most of the validation code being implemented there, but it wasn't itself vulnerable; instead, it gained the feature that some other vulnerable crates used to fix the vulnerability in themselves. Crates like gix-validate are not listed in the GHSA advisories because they are not themselves vulnerable.

I do not propose to remove any crates from any GHSA advisories, aside from removing gix-fs from the one to which it does not apply. By the GHSA advisories I mean the repository-local advisories and GitHub Advisory Database global advisories. I believe, in the context of those advisories, that all the listed crates, other than gix-fs for one of the vulnerabilities, are correct or at least reasonable.

Since some code did have to change in more crates than the few I've proposed RUSTSEC advisories for, should I look into possibly adding RUSTSEC advisories for all crates listed in GHSA-7w47-3wg8-547c, and in GHSA-49jc-r788-3fc9 (except gix-fs)? Unlike GHSA advisories, RUSTSEC advisories cannot list multiple crates, so each crate needs its own copy of the advisory, which is why I've been reluctant to do such a thing.

However, it may be that such a fix is automatically inherited by fixed upstream crates in which case it probably doesn't have to be listed.

My specific thinking, which is an effort to apply something like the guidance in rustsec/advisory-db#1703 (comment) and rustsec/advisory-db#1705 (comment) to the present, admittedly different situation, is that:

1. If a crate A's code is only changed to make it compatible with another crate B where the vulnerability is fixed, and B will have a RUSTSEC advisory, and A depends on B, and no unpatched version of A can be installed together with any patched version of B, then it should be okay for A not to have a RUSTSEC advisory.
2. This is even though some versions of A really are vulnerable--if you specified them when installing and didn't pin B, you'd get vulnerable versions of both A and B, due to dependency version requirements accurately expressing that A had to be changed to allow the fix in B to work (i.e., part of the change to fix the vulnerability really was in A). Thus listing both A and B still is justified in both repo-local and global GHSA advisories.
3. The difference in RUSTSEC advisories is that the entire advisory actually has to be duplicated (i.e., where each crate has to have its own separate advisory file and assigned RUSTSEC ID). So omitting some crates' RUSTSEC advisories for a vulnerability, so long as other RUSTSEC advisories for the same vulnerability will always be applicable, is sometimes justified.

If any of those three points is not correct, then my reasoning should not be accepted.

Another, perhaps more important consideration is whether it is really true that no unpatched version of gix-worktree-state can be installed together with a patched version of gix-worktree. If it's possible to have a patched version of gix-worktree but an unpatched version of gix-worktree-state installed along with it that causes gix-worktree to not use its validation features, then that would definitely require gix-worktree-state to be treated as a primary affected crate.

The version of gix-worktree-state with the patch was v0.11.0. Its changes were not limited to dependencies. But its dependencies included these changes:

-gix-worktree = { version = "^0.33.1", path = "../gix-worktree", default-features = false, features = ["attributes"] }
-gix-index = { version = "^0.32.1", path = "../gix-index" }
-gix-fs = { version = "^0.10.2", path = "../gix-fs" }
+gix-worktree = { version = "^0.34.0", path = "../gix-worktree", default-features = false, features = ["attributes"] }
+gix-index = { version = "^0.33.0", path = "../gix-index" }
+gix-fs = { version = "^0.11.0", path = "../gix-fs" }

As I understand it, when SemVer is extended with the ^ and ~ notation, the ^ has a special meaning for ^0.Y.Z, such that it is equivalent to ~0.Y.Z, requiring an exact match for Y. (This differs from the meaning of ^X.Y.Z with nonzero X, which also matches higher values of Y and is thus not equivalent to ~X.Y.Z.) Assuming that is the case and cargo follows this rule, the previous version of gix-worktree-state cannot be installed together with the patched versions of gix-worktree, gix-index, or gix-fs.

Thus, so long as there are RUSTSEC advisories for them--or at least one of them--omitting a RUSTSEC advisory for gix-worktree-state should not cause any vulnerable Cargo.toml or Cargo.lock in any project to be wrongly detected as non-vulnerable.

(The same does not apply for having RUSTSEC advisories for gix-worktree-state instead of those other crates, because a project can have those other crates as dependencies without even indirectly having gix-worktree-state as a dependency.)

This is assuming cargo does treat ^0. in this way. Neither the cargo documentation on Specifying Dependencies, nor the SemVer standards to which I believe ~ and ^ are extensions, seem to speak to this explicitly, though I could be missing something.

[Byron]

I think your reasoning is correct. And it's indeed a pitty that it's currently impossible to specify multiple affected crates with the same advisory.