Headline

CVE-2021-28879: Fix underflow in specialized ZipImpl::size_hint by SkiFire13 · Pull Request #82289 · rust-lang/rust

In the standard library in Rust before 1.52.0, the Zip implementation can report an incorrect size due to an integer overflow. This bug can lead to a buffer overflow when a consumed Zip iterator is used again.

3 years ago

CVE

Open in Source

#mac #git #buffer_overflow #auth

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

Conversation 17 Commits 3 Checks 10 Files changed

Conversation

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@kennytm is that supposed to be the same as an r+? Sorry, but this is my first time seeing that in this repository

Copy link

Contributor

****Qwaz** commented Feb 19, 2021**

I’m not sure if this fix interacts well with next_back()'s length handling. Say we are using iterators with length 1 for both A and B.

Initialize zip

index = 0, len = 1

Call next_back()

index = 0, len = 0
get_unchecked(0) is called on both A and B

Call next()

Since self.index < self.a.size(), get_unchecked(0) is called again on A
index = 1, len = 1

Calling get_unchecked() on the same index twice is a violation of the second safety invariant of TrustedRandomAccess and can lead to duplicated elements.

Copy link

Contributor

****Qwaz** commented Feb 19, 2021**

Oh, was this already possible even before the fix?

Oh, was this already possible even before the fix?

Yes, it was and this PR wasn’t aimed to fix it. I’ve already opened #82291 a couple of hours ago showing how it can be exploited and #82292 to fix it, unfortunately it conflicts with this PR (both add a test at the end of the same file).

Copy link

Member

****bluss** commented Feb 20, 2021 •**

It’s worth considering if the fix that moved the increment of index can be reverted.

Alternatively - would it be better to rewrite the whole zip specialization in terms of a “next_unchecked” method instead, and would that be more robust?

It’s worth considering if the fix that moved the increment of index can be reverted.

What does that have to do with the bug this PR should fix?

Alternatively - would it be better to rewrite the whole zip specialization in terms of a “next_unchecked” method instead, and would that be more robust?

A discussion about that started here, then we moved on zulip (here). Anyway I thought you already ruled out a next_unchecked approach because LLVM had more trouble to optimize it (and I think it still has).

Copy link

Member

****bluss** commented Feb 20, 2021**

What does that have to do with the bug this PR should fix?

Hm, oh I thought this issue was directly caused by that, but on new reading you’re right, it’s not related, sorry about that.

Anything from 2016 could of course be reevaluated 🙂

m-ou-se added A-iterators

Area: Iterators

T-libs

Relevant to the library team, which will review and decide on the PR/issue.

S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Mar 3, 2021

@rustbot label -S-waiting-on-author +S-waiting-on-review

rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

and removed S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Mar 3, 2021

Copy link

Contributor

****bors** commented Mar 4, 2021**

📌 Commit aeb4ea7 has been approved by m-ou-se

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

Mar 4, 2021

m-ou-se added a commit to m-ou-se/rust that referenced this pull request

Mar 5, 2021

This was referenced

Mar 5, 2021

bors added a commit to rust-lang-ci/rust that referenced this pull request

Mar 5, 2021

JohnTitor added a commit to JohnTitor/rust that referenced this pull request

Mar 6, 2021