Skip to content

Commit

Permalink
Add support for Span in the user-after-pop-front detector (#54)
Browse files Browse the repository at this point in the history 
* Rename the detector to

* Update for new taint api

---------

Co-authored-by: Simone <simone.monica@trailofbits.com>
  • Loading branch information
@tarunbhm @smonicas
tarunbhm and smonicas authored Jan 24, 2024
1 parent b6100dd commit 23c987e
Show file tree
Hide file tree
Showing 7 changed files with 481 additions and 216 deletions.
2 changes: 1 addition & 1 deletion README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ Num | Detector | What it Detects | Impact | Confidence | Cairo
10 | `reentrancy-benign` | Detect when a storage variable is written after an external call but not read before | Low | Medium | 1 & 2
11 | `reentrancy-events` | Detect when an event is emitted after an external call leading to out-of-order events | Low | Medium | 1 & 2
12 | `dead-code` | Private functions never used | Low | Medium | 1 & 2
13 | `array-use-after-pop-front` | Detect use of an array after removing element(s) | Low | Medium | 1 & 2
13 | `use-after-pop-front` | Detect use of an array or a span after removing element(s) | Low | Medium | 1 & 2

The Cairo column represent the compiler version(s) for which the detector is valid.

Expand Down
4 changes: 2 additions & 2 deletions src/detectors/mod.rs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
use self::detector::Detector;

pub mod array_use_after_pop_front;
pub mod controlled_library_call;
pub mod dead_code;
pub mod detector;
Expand All @@ -13,10 +12,11 @@ pub mod unchecked_l1_handler_from;
pub mod unused_arguments;
pub mod unused_events;
pub mod unused_return;
pub mod use_after_pop_front;

pub fn get_detectors() -> Vec<Box<dyn Detector>> {
vec![
Box::<array_use_after_pop_front::ArrayUseAfterPopFront>::default(),
Box::<use_after_pop_front::UseAfterPopFront>::default(),
Box::<controlled_library_call::ControlledLibraryCall>::default(),
Box::<unused_events::UnusedEvents>::default(),
Box::<dead_code::DeadCode>::default(),
Expand Down
160 changes: 130 additions & 30 deletions src/detectors/array_use_after_pop_front.rs → src/detectors/use_after_pop_front.rs
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::collections::HashSet;

use super::detector::{Confidence, Detector, Impact, Result};
use crate::analysis::taint::WrapperVariable;
use crate::core::compilation_unit::CompilationUnit;
Expand All @@ -7,18 +9,31 @@ use cairo_lang_sierra::extensions::array::ArrayConcreteLibfunc;
use cairo_lang_sierra::extensions::core::{CoreConcreteLibfunc, CoreTypeConcrete};
use cairo_lang_sierra::program::{GenStatement, Statement as SierraStatement};
use fxhash::FxHashSet;
use std::collections::HashSet;

#[derive(Default)]
pub struct ArrayUseAfterPopFront {}
pub struct UseAfterPopFront {}

impl Detector for ArrayUseAfterPopFront {
enum CollectionType {
Array,
Span,
}

impl CollectionType {
fn is_array(&self) -> bool {
match self {
CollectionType::Array => true,
CollectionType::Span => false,
}
}
}

impl Detector for UseAfterPopFront {
fn name(&self) -> &str {
"array-use-after-pop-front"
"use-after-pop-front"
}

fn description(&self) -> &str {
"Detect use of an array after removing element(s)"
"Detect use of an array or a span after removing element(s)"
}

fn confidence(&self) -> Confidence {
Expand All @@ -35,7 +50,7 @@ impl Detector for ArrayUseAfterPopFront {

for compilation_unit in compilation_units.iter() {
for function in compilation_unit.functions_user_defined() {
let pop_fronts: Vec<(usize, WrapperVariable)> = function
let pop_fronts: Vec<(usize, WrapperVariable, CollectionType)> = function
.get_statements()
.iter()
.enumerate()
Expand All @@ -51,31 +66,55 @@ impl Detector for ArrayUseAfterPopFront {
Some((
index,
WrapperVariable::new(function.name(), invoc.args[0].id),
CollectionType::Array,
))
}
CoreConcreteLibfunc::Array(
ArrayConcreteLibfunc::SnapshotPopFront(_),
) => Some((
index,
WrapperVariable::new(function.name(), invoc.args[0].id),
CollectionType::Span,
)),
CoreConcreteLibfunc::Array(
ArrayConcreteLibfunc::SnapshotPopBack(_),
) => Some((
index,
WrapperVariable::new(function.name(), invoc.args[0].id),
CollectionType::Span,
)),
_ => None,
}
}
_ => None,
})
.collect();

let bad_array_used: Vec<&(usize, WrapperVariable)> = pop_fronts
.iter()
.filter(|(index, bad_array)| {
self.is_array_used_after_pop_front(
compilation_unit,
function,
bad_array,
*index,
)
})
.collect();
// Required to silence clippy too-complex-type warning
type BadCollectionType<'a, 'b> = Vec<(&'a WrapperVariable, &'b CollectionType)>;

let (bad_array_used, bad_span_used): (BadCollectionType, BadCollectionType) =
pop_fronts
.iter()
.filter_map(|(index, bad_array, collection_type)| {
let is_used = self.is_used_after_pop_front(
compilation_unit,
function,
bad_array,
*index,
);
if is_used {
Some((bad_array, collection_type))
} else {
None
}
})
.partition(|(_, collection_type)| collection_type.is_array());

if !bad_array_used.is_empty() {
let array_ids = bad_array_used
.iter()
.map(|f| f.1.variable())
.map(|f| f.0.variable())
.collect::<Vec<u64>>();
let message = match array_ids.len() {
1 => format!(
Expand All @@ -96,15 +135,40 @@ impl Detector for ArrayUseAfterPopFront {
message,
});
}

if !bad_span_used.is_empty() {
let span_ids = bad_span_used
.iter()
.map(|f| f.0.variable())
.collect::<Vec<u64>>();
let message = match span_ids.len() {
1 => format!(
"The span {:?} is used after removing elements from it in the function {}",
span_ids,
&function.name()
),
_ => format!(
"The spans {:?} are used after removing elements from them in the function {}",
span_ids,
&function.name()
)
};
results.insert(Result {
name: self.name().to_string(),
impact: self.impact(),
confidence: self.confidence(),
message,
});
}
}
}

results
}
}

impl ArrayUseAfterPopFront {
fn is_array_used_after_pop_front(
impl UseAfterPopFront {
fn is_used_after_pop_front(
&self,
compilation_unit: &CompilationUnit,
function: &Function,
Expand Down Expand Up @@ -204,8 +268,8 @@ impl ArrayUseAfterPopFront {

// check if the bad array is returned by the function
// if yes then check if its a loop function
// if not then its clear usage of a bad array
// if yes then we need to check its caller to see if it uses the bad array
// if not then its clear usage of a bad array
fn check_returns(
&self,
compilation_unit: &CompilationUnit,
Expand All @@ -230,18 +294,19 @@ impl ArrayUseAfterPopFront {
// We can not find the array from the return types of the function
// We assume that the array is returned at the same index as it was taken on
let return_array_indices: Vec<usize> = function
.params()
.params_all()
.enumerate()
.filter_map(|(i, param)| {
let param_type = compilation_unit
.registry()
.get_type(&param.ty)
.expect("Type not found in the registry");

if let CoreTypeConcrete::Array(_) = param_type {
return Some(i);
match param_type {
CoreTypeConcrete::Array(_) => Some(i),
span if self.is_core_type_concrete_span(compilation_unit, span) => Some(i),
_ => None,
}
None
})
.collect();

Expand Down Expand Up @@ -310,10 +375,11 @@ impl ArrayUseAfterPopFront {
.get_type(r)
.expect("Type not found in the registry");

if let CoreTypeConcrete::Array(_) = return_type {
return Some(i);
match return_type {
CoreTypeConcrete::Array(_) => Some(i),
span if self.is_core_type_concrete_span(compilation_unit, span) => Some(i),
_ => None,
}
None
})
.collect();

Expand All @@ -322,8 +388,9 @@ impl ArrayUseAfterPopFront {
return false;
}

// Not sure if it is required because taint analysis adds all the arguments as
// tainters of the all the return values.
// It is not required because taint analysis adds all the arugments as
// tainters of the all the return values. Added it in case the taint
// analysis is improved later on to be more granular.
let returned_bad_arrays: Vec<WrapperVariable> = function
.get_statements()
.iter()
Expand All @@ -342,4 +409,37 @@ impl ArrayUseAfterPopFront {

!returned_bad_arrays.is_empty()
}

// The Span is not a Core Sierra type, it is defined in the corelib as a Struct
// Therefore we can not match it against CoreTypeConcrete::Span directly
fn is_core_type_concrete_span(
&self,
compilation_unit: &CompilationUnit,
maybe_span: &CoreTypeConcrete,
) -> bool {
match maybe_span {
CoreTypeConcrete::Struct(struct_type) => match &struct_type.members[..] {
[maybe_snapshot, ..] => {
let maybe_snapshot_type = compilation_unit
.registry()
.get_type(maybe_snapshot)
.expect("Type not found in the registry");

match maybe_snapshot_type {
CoreTypeConcrete::Snapshot(maybe_array) => {
let maybe_array_type = compilation_unit
.registry()
.get_type(&maybe_array.ty)
.expect("Type not found in the registry");

matches!(maybe_array_type, CoreTypeConcrete::Array(_))
}
_ => false,
}
}
_ => false,
},
_ => false,
}
}
}
Loading

0 comments on commit 23c987e

Please sign in to comment.