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.

Sign up for GitHub

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

Jump to bottom

upgrade gitoxide to v0.54 #12731

Merged
merged 1 commit into from
Sep 25, 2023
Merged

upgrade gitoxide to v0.54 #12731

merged 1 commit into from
Sep 25, 2023

Conversation

Byron
Copy link
Member

@Byron Byron commented Sep 24, 2023 •
edited
Loading

This reduces the binary size and fixes an exploitable bug that could allow
code execution by injection arguments into hostnames of ssh URLs.

Binary Sizes (Release)
  • master: 27930520
  • this branch: 27869304 - a whopping 61216B less. I assume this will get worse again once more functionality will be used in future PRs.
Possible Vulnerability

In versions prior to v0.54, running the following would cause the calculator app to be started on MacOS:

❯ gix clone 'ssh://-oProxyCommand=open$IFS-aCalculator/foo'

Now it prints Error: Host name '-oProxyCommand=open-aCalculator' could be mistaken for a command-line argument.

Given the nature of builds with cargo and the availability of build scripts, I think cargo isn't prone to this issue. However, I thought it was good to upgrade anyway.

Please note that a CVE doesn't exist yet, but I will check with Rustsec on how to proceed with this.
CC @Shnatsel

Tasks

  • fix tests - the bug was introduced in GitoxideLabs/gitoxide@74ce863 and it leads the local symref refs/remotes/origin/HEAD to point to a non-existing branch.
    • This is a feature, but one with the shortcoming that it's allowed to point to a none-existing ref, and that ref is not automatically created, yet, and it's not covered by a refspec. Previously it would just let the symbolic ref point to the peeled object that is known.

@rustbot
Copy link
Collaborator

rustbot commented Sep 24, 2023

r? @weihanglo

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 24, 2023
@Shnatsel
Copy link
Member

At a glance it does deserve a security advisory, and I would be glad to help draft a RUSTSEC one. I believe something that clones untrusted URLs can end up with arbitrary code execution, is that correct?

@Byron
Copy link
Member Author

Byron commented Sep 24, 2023

Yes, that's correct. Here is more details about how this can be abused: https://secure.phabricator.com/T12961 .

I also sent an email to Rust-Sec (security@rust-lang) directly BTW, in case it helps to avoid duplication of effort, otherwise your help would definitely be appreciated.

@Shnatsel
Copy link
Member

If possible, a point release for the previous series with the fix would also be great, so that affected API users could simply cargo update to fix the issue instead of requiring them to adapt to the API changes.

@Byron
Copy link
Member Author

Byron commented Sep 24, 2023

If possible, a point release for the previous series with the fix would also be great, so that affected API users could simply cargo update to fix the issue instead of requiring them to adapt to the API changes.

I think there is no API change, at least not for most, that's the way gix is being released that causes breaking changes upstream to indicate breaking changes downstream (within gix-*) for safety as it conservatively assumes these changes are exposed in some way. gitoxide users know that, and I'd hope a CVE helps to inform all of them.

In practice, I think 'only' cargo-audit and tame-index have a chance to run into this issue.

@Shnatsel
Copy link
Member

The presence of a point release defines how many packages will need to release new versions with the fix, and how many of these packages then need to be picked up by Linux distributions, etc. to get the issue fixed.

Is it possible to take the 0.53.1 release, apply only the patch for this one issue, and release it as 0.53.2? That would be ideal.

@Byron
Copy link
Member Author

Byron commented Sep 24, 2023

Technically, gix-transport is the crate that saw the fix, but unfortunately it is legitimately breaking as it changes an Error struct to an enum. In theory, I could release that as patch probably rightfully assuming that nobody will be broken by it as nobody matched on that particular error.

If this is what you'd suggest doing, and I support that choice, I can make it happen and all users of gix 0.53 will get the upgrade more easily.

@Shnatsel
Copy link
Member

Could you release a minimum fix that does not properly report errors, but is semver-compatible? I believe that is warranted for a security patch. It is fine to leave proper error reporting to 0.54.

@Byron
Copy link
Member Author

Byron commented Sep 25, 2023

Ok, a new version of gix-transport, v0.36.1, has just been released with a semver compatible backport. This will fix gix v0.53 as well.

@Byron
upgrade gitoxide to v0.54
3f7d556 
This reduces the binary size and fixes an exploitable bug that could allow
code execution by injection arguments into hostnames of ssh URLs.
@Byron Byron marked this pull request as ready for review September 25, 2023 12:56
weihanglo
weihanglo approved these changes Sep 25, 2023
View reviewed changes
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

This looks pretty good to me. Thank you all for taking care of security!

@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Sep 25, 2023

📌 Commit 3f7d556 has been approved by weihanglo

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 25, 2023
@bors
Copy link
Contributor

bors commented Sep 25, 2023

⌛ Testing commit 3f7d556 with merge 4a470d5...

@bors
Copy link
Contributor

bors commented Sep 25, 2023

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 4a470d5 to master...

@bors bors merged commit 4a470d5 into rust-lang:master Sep 25, 2023
21 checks passed
@weihanglo weihanglo mentioned this pull request Sep 27, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 27, 2023
@bors
Auto merge of rust-lang#116189 - weihanglo:update-cargo, r=weihanglo
d206f2c 
Update cargo

11 commits in 414d9e3a6d8096f3e276234ce220c868767a8792..e6aabe8b3fcf639be3a5bf68e77853bd7b3fa27d
2023-09-22 07:03:57 +0000 to 2023-09-26 16:31:53 +0000
- Use full target spec for `cargo rustc --print --target` (rust-lang/cargo#12743)
- feat(embedded): Hack in code fence support (rust-lang/cargo#12681)
- chore(ci): Update Renovate schema (rust-lang/cargo#12741)
- more specific registry index not found msg (rust-lang/cargo#12732)
- docs: warn about upload timeout (rust-lang/cargo#12733)
- Fix some typos (rust-lang/cargo#12730)
- upgrade gitoxide to v0.54 (rust-lang/cargo#12731)
- Update target-arch-aware crates to support mips r6 targets (rust-lang/cargo#12720)
- Buffer console status messages. (rust-lang/cargo#12727)
- Fix spurious errors with networking tests. (rust-lang/cargo#12726)
- refactor(SourceId): merge `name` and `alt_registry_key` into one enum (rust-lang/cargo#12675)

r? ghost
@ehuss ehuss added this to the 1.74.0 milestone Sep 30, 2023
@nspin nspin mentioned this pull request Oct 3, 2023
Closed
@klensy klensy mentioned this pull request Jun 27, 2024
Merged
bors added a commit that referenced this pull request Jun 27, 2024
@bors
Auto merge of #14160 - klensy:gix-rev, r=ehuss
942536c 
gix: remove `revision` feature from cargo

Feature was added in #12731, but cargo builds without it and looks like used in tests only, so unset it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weihanglo weihanglo weihanglo approved these changes

Assignees

@weihanglo weihanglo

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.74.0
Development

Successfully merging this pull request may close these issues.
6 participants
@Byron @rustbot @Shnatsel @weihanglo @bors @ehuss