Fix soundness issue for `replace_range` and `range` #81169

dylni · 2021-01-18T15:33:15Z

Fixes #81138 by only calling start_bound and end_bound once.

I also fixed the same issue for BTreeMap::range and BTreeSet::range.

rust-highfive · 2021-01-18T15:33:18Z

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

bors · 2021-01-18T21:04:32Z

☔ The latest upstream changes (presumably #81159) made this pull request unmergeable. Please resolve the merge conflicts.

bugadani · 2021-01-18T21:40:37Z

library/alloc/src/string.rs

@@ -1553,18 +1553,21 @@ impl String {
        // Replace_range does not have the memory safety issues of a vector Splice.
        // of the vector version. The data is just plain bytes.

-        match range.start_bound() {
+        let start = range.start_bound();


I think comments explaining this change might be beneficial here as well. With or without a big // WARNING: do not inline this variable :)

bugadani · 2021-01-18T21:41:31Z

library/alloc/src/string.rs

            Included(&n) => assert!(self.is_char_boundary(n + 1)),
            Excluded(&n) => assert!(self.is_char_boundary(n)),
            Unbounded => {}
        };

-        unsafe { self.as_mut_vec() }.splice(range, replace_with.bytes());
+        // Using `range` again would be unsound (#81138)


Ah, here it is. Hmm.

@bugadani Do you think another comment would be helpful? I added one where I thought it would make the most sense, since adding one above let start but not let end is a little weird.

I think more comments wouldn't hurt 🙂

That's true 😛

KodrAus · 2021-01-19T01:22:31Z

@dylni Would you like to include the example from #81138 as a test case to make sure we are still maintaining the UTF8 invariant?

KodrAus · 2021-01-19T01:28:33Z

library/alloc/src/collections/btree/navigate.rs

@@ -25,7 +25,10 @@ where
    K: Borrow<Q>,
    R: RangeBounds<Q>,
 {
-    match (range.start_bound(), range.end_bound()) {
+    // Using `range` again would be unsound (#81138)


Suggested change

// Using `range` again would be unsound (#81138)

// Using `range` again would be unsound (#81138)

// We assume the bounds reported by `range` remain the same, but

// an adversarial implementation could change between calls

library/alloc/src/string.rs

KodrAus · 2021-01-19T01:29:54Z

I've just left a few comments. It would be good to include the EvilRange as a test case, but r=me at any time.

dylni · 2021-01-19T01:57:29Z

Would you like to include the example from #81138 as a test case to make sure we are still maintaining the UTF8 invariant?

Yes, good idea.

KodrAus · 2021-01-19T02:56:37Z

@dylni This looks good to me! Would you like to squash these down then we should be good to go.

dylni · 2021-01-19T03:18:04Z

Thanks @KodrAus! Is this what you had in mind? Squashing is a little awkward with merge commits.

KodrAus · 2021-01-19T05:33:50Z

@bors r+ p=1

bors · 2021-01-19T05:33:51Z

📌 Commit b96063c has been approved by KodrAus

KodrAus · 2021-01-19T05:34:51Z

That's it, thanks! A rebase and squash is what I'd meant. We try to avoid merge commits in PRs and leave that to bors to handle.

Thanks for fixing this up 🙇

bors · 2021-01-19T05:58:56Z

⌛ Testing commit b96063c with merge 7d7b22d...

bors · 2021-01-19T08:59:30Z

☀️ Test successful - checks-actions
Approved by: KodrAus
Pushing 7d7b22d to master...

dylni · 2021-01-19T13:15:48Z

Oh, thanks, that makes sense.

ssomers · 2021-01-19T13:38:32Z

I also fixed the same issue for BTreeMap::range and BTreeSet::range.

Useful and inspirational, but I feel the need to point out that you didn't entirely plug that hole, and probably nobody can. Unlike String::replace_range whose range is tied to usize, the caller of BTreeMap::range gets to choose a type and its Ord implementation. I got a little lost trying to brew a really evil example with a panic, but I still believe it can be done. Here on playground is proof that there are 12 integers from 5 to 7, with the help of an evil borrow.

dylni · 2021-01-22T04:29:45Z

@ssomers I think that's ok. If an impl used by BTreeMap misbehaves, BTreeMap is allowed to misbehave too. The only thing it can't do is something unsound. Although the playground example doesn't give the right result for the number values, it gives the right result based on the Borrow impl.

ssomers · 2021-01-22T09:34:32Z

@dylni I agree, but that means there was no unsoundness in BTreeMap's use of start_bound/end_bound, is what I realized this week. How many times these functions are called or what they return doesn't matter for unsoundness. It's what the various invocations of the lookup type's Ord say that might make BTreeMap trip up.

This PR of course still includes a welcome fix to avoid BTreeMap from appearing to misbehave when the range misbehaves. If there was a possibility of unsoundness in BTreeMap, that possibility is still there. And I can't find it.

dylni · 2021-01-23T16:36:21Z

@ssomers You're right. I was looking at the unsafe code here, but it's inside a safe function, so it makes sense that this can't be exploited.

ssomers · 2021-01-23T17:29:16Z

Not sure how you arrived in left_edge. But yes, disappointingly, I don't think there's anything to exploit. Essentially, BTreeMap is already prepared to withstand a malicious implementation of Ord for the key type, so the fact that range smuggles in another Ord for its search can't make things worse. When BTreeMap validates the range and panics, it doesn't do that to protect itself from unsoundness, but just to advise the caller that they messed up the range (with a message assuming they have an honest Ord implementation). Not sure if the original authors of the code realized that, but it's written in the contract that it should panic on an invalid range, so I'd better leave that to be.

dylni · 2021-01-23T17:53:11Z

@ssomers left_edge is called here.

When BTreeMap validates the range and panics, it doesn't do that to protect itself from unsoundness, but just to advise the caller that they messed up the range (with a message assuming they have an honest Ord implementation).

I agree, but this was surprising. I think I'll change the comments a little there to make it clearer for people reading it in the future.

…ark-Simulacrum BTreeMap: test all borrowing interfaces and test more chaotic order behavior Inspired by rust-lang#81169, test what happens if you mess up order of the type with which you search (as opposed to the key type). r? `@Mark-Simulacrum`

…ents, r=m-ou-se Clarify BTree `range_search` comments These comments were added by rust-lang#81169. However, the soundness issue [might not be exploitable here](rust-lang#81169 (comment)), so the comments should be updated. cc `@ssomers`

…-ou-se Clarify BTree `range_search` comments These comments were added by #81169. However, the soundness issue [might not be exploitable here](rust-lang/rust#81169 (comment)), so the comments should be updated. cc `@ssomers`

rust-highfive assigned withoutboats Jan 18, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 18, 2021

camelid added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jan 18, 2021

bugadani reviewed Jan 18, 2021

View reviewed changes

KodrAus reviewed Jan 19, 2021

View reviewed changes

library/alloc/src/string.rs Show resolved Hide resolved

Fix soundness issue for replace_range and range

b96063c

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 Jan 19, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 19, 2021

bors merged commit 7d7b22d into rust-lang:master Jan 19, 2021

rustbot added this to the 1.51.0 milestone Jan 19, 2021

This was referenced Jan 19, 2021

BTree: add drain methods #81075

Closed

BTreeMap: split up range_search into two stages #81094

Merged

ssomers mentioned this pull request Jan 19, 2021

BTreeMap: test all borrowing interfaces and test more chaotic order behavior #81191

Merged

dylni mentioned this pull request Jan 23, 2021

Clarify BTree range_search comments #81312

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix soundness issue for `replace_range` and `range` #81169

Fix soundness issue for `replace_range` and `range` #81169

dylni commented Jan 18, 2021

rust-highfive commented Jan 18, 2021

bors commented Jan 18, 2021

bugadani Jan 18, 2021

bugadani Jan 18, 2021

dylni Jan 18, 2021

KodrAus Jan 19, 2021

dylni Jan 19, 2021

KodrAus commented Jan 19, 2021

KodrAus Jan 19, 2021

KodrAus commented Jan 19, 2021

dylni commented Jan 19, 2021

KodrAus commented Jan 19, 2021

dylni commented Jan 19, 2021

KodrAus commented Jan 19, 2021

bors commented Jan 19, 2021

KodrAus commented Jan 19, 2021 •

edited

Loading

bors commented Jan 19, 2021

bors commented Jan 19, 2021

dylni commented Jan 19, 2021

ssomers commented Jan 19, 2021

dylni commented Jan 22, 2021

ssomers commented Jan 22, 2021 •

edited

Loading

dylni commented Jan 23, 2021

ssomers commented Jan 23, 2021

dylni commented Jan 23, 2021

Fix soundness issue for replace_range and range #81169

Fix soundness issue for replace_range and range #81169

Conversation

dylni commented Jan 18, 2021

rust-highfive commented Jan 18, 2021

bors commented Jan 18, 2021

bugadani Jan 18, 2021

Choose a reason for hiding this comment

bugadani Jan 18, 2021

Choose a reason for hiding this comment

dylni Jan 18, 2021

Choose a reason for hiding this comment

KodrAus Jan 19, 2021

Choose a reason for hiding this comment

dylni Jan 19, 2021

Choose a reason for hiding this comment

KodrAus commented Jan 19, 2021

KodrAus Jan 19, 2021

Choose a reason for hiding this comment

KodrAus commented Jan 19, 2021

dylni commented Jan 19, 2021

KodrAus commented Jan 19, 2021

dylni commented Jan 19, 2021

KodrAus commented Jan 19, 2021

bors commented Jan 19, 2021

KodrAus commented Jan 19, 2021 • edited Loading

bors commented Jan 19, 2021

bors commented Jan 19, 2021

dylni commented Jan 19, 2021

ssomers commented Jan 19, 2021

dylni commented Jan 22, 2021

ssomers commented Jan 22, 2021 • edited Loading

dylni commented Jan 23, 2021

ssomers commented Jan 23, 2021

dylni commented Jan 23, 2021

Fix soundness issue for `replace_range` and `range` #81169

Fix soundness issue for `replace_range` and `range` #81169

KodrAus commented Jan 19, 2021 •

edited

Loading

ssomers commented Jan 22, 2021 •

edited

Loading