Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

removes raw indexing into packet data #25554

Merged
merged 1 commit into from
Jun 3, 2022

Conversation

behzadnouri
Copy link
Contributor

Problem

Packets are at the boundary of the system where, vast majority of the
time, they are received from an untrusted source. Raw indexing into the
data buffer can open attack vectors if the offsets are invalid.
Validating offsets beforehand is verbose and error prone.

Summary of Changes

The commit updates Packet::data() api to take a SliceIndex and always to
return an Option. The call-sites are so forced to explicitly handle the
case where the offsets are invalid.

@behzadnouri behzadnouri force-pushed the packet-get-data branch 2 times, most recently from 6ccc45a to f764703 Compare May 25, 2022 16:53
@codecov
Copy link

codecov bot commented May 25, 2022

Codecov Report

Merging #25554 (fcfacc1) into master (f9f6b94) will increase coverage by 0.0%.
The diff coverage is 74.0%.

@@           Coverage Diff            @@
##           master   #25554    +/-   ##
========================================
  Coverage    82.0%    82.0%            
========================================
  Files         655      626    -29     
  Lines      171822   170976   -846     
  Branches      335        0   -335     
========================================
- Hits       140972   140344   -628     
+ Misses      30734    30632   -102     
+ Partials      116        0   -116     

@jstarry
Copy link
Contributor

jstarry commented May 26, 2022

I had a feeling this was going to be a follow up change :)

@KirillLykov
Copy link
Contributor

It looks like packet.data() slicing is also used in this new PR

@behzadnouri behzadnouri force-pushed the packet-get-data branch 3 times, most recently from 3ae9c5a to 2bdf87c Compare May 26, 2022 14:56
@behzadnouri behzadnouri requested a review from jstarry May 26, 2022 15:11
@behzadnouri behzadnouri force-pushed the packet-get-data branch 2 times, most recently from 790f3da to 5d5e81c Compare June 2, 2022 13:19
@behzadnouri
Copy link
Contributor Author

@jstarry any objection to merging this?

@jstarry
Copy link
Contributor

jstarry commented Jun 2, 2022

@jstarry any objection to merging this?

No, feel free to merge

Packets are at the boundary of the system where, vast majority of the
time, they are received from an untrusted source. Raw indexing into the
data buffer can open attack vectors if the offsets are invalid.
Validating offsets beforehand is verbose and error prone.

The commit updates Packet::data() api to take a SliceIndex and always to
return an Option. The call-sites are so forced to explicitly handle the
case where the offsets are invalid.
@behzadnouri behzadnouri merged commit 5dbf7d8 into solana-labs:master Jun 3, 2022
@behzadnouri behzadnouri deleted the packet-get-data branch June 3, 2022 01:05
mergify bot pushed a commit that referenced this pull request Jun 20, 2022
Packets are at the boundary of the system where, vast majority of the
time, they are received from an untrusted source. Raw indexing into the
data buffer can open attack vectors if the offsets are invalid.
Validating offsets beforehand is verbose and error prone.

The commit updates Packet::data() api to take a SliceIndex and always to
return an Option. The call-sites are so forced to explicitly handle the
case where the offsets are invalid.

(cherry picked from commit 5dbf7d8)

# Conflicts:
#	ledger/src/shred.rs
#	ledger/src/sigverify_shreds.rs
#	perf/src/sigverify.rs
mergify bot added a commit that referenced this pull request Jun 20, 2022
* removes raw indexing into packet data (#25554)

Packets are at the boundary of the system where, vast majority of the
time, they are received from an untrusted source. Raw indexing into the
data buffer can open attack vectors if the offsets are invalid.
Validating offsets beforehand is verbose and error prone.

The commit updates Packet::data() api to take a SliceIndex and always to
return an Option. The call-sites are so forced to explicitly handle the
case where the offsets are invalid.

(cherry picked from commit 5dbf7d8)

# Conflicts:
#	ledger/src/shred.rs
#	ledger/src/sigverify_shreds.rs
#	perf/src/sigverify.rs

* removes mergify merge conflicts

Co-authored-by: behzad nouri <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants