Skip to content

Commit

Permalink
[taint] Match potential tainted values wrt field matchers, taint the …
Browse files Browse the repository at this point in the history
…rhs of Sil.Store

Summary:
Here we add functionality for matching a field name against a field matcher and tainting the corresponding `rhs` value of the `Sil.Store` instruction.

Better error message and more tests in upcoming diffs.

Reviewed By: geralt-encore

Differential Revision: D45869653

fbshipit-source-id: c717d4cbdb527905d75f118240fbc3dd1c0a51c2
  • Loading branch information
dulmarod authored and facebook-github-bot committed May 17, 2023
1 parent 78f28b2 commit 109e7a1
Show file tree
Hide file tree
Showing 9 changed files with 393 additions and 167 deletions.
5 changes: 5 additions & 0 deletions infer/src/pulse/Pulse.ml
Original file line number Diff line number Diff line change
Expand Up @@ -954,6 +954,11 @@ module PulseTransferFunctions = struct
List.concat_map ls_astate_lhs_addr_hist ~f:(fun result ->
let<*> astate, lhs_addr_hist = result in
let<**> astate = and_is_int_if_integer_type typ rhs_addr astate in
let<*> astate =
PulseTaintOperations.store tenv path loc ~lhs:lhs_exp
~rhs:(rhs_exp, (rhs_addr, hist), typ)
astate
in
[ PulseOperations.write_deref path loc ~ref:lhs_addr_hist ~obj:(rhs_addr, hist)
astate ] )
in
Expand Down
2 changes: 2 additions & 0 deletions infer/src/pulse/PulseTaintItem.mli
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ type value =

type t = {kinds: TaintConfig.Kind.t list; value: value; origin: origin} [@@deriving compare, equal]

val pp_value : F.formatter -> value -> unit

val pp_value_plain : F.formatter -> value -> unit

val pp : F.formatter -> t -> unit
333 changes: 195 additions & 138 deletions infer/src/pulse/PulseTaintItemMatcher.ml

Large diffs are not rendered by default.

6 changes: 3 additions & 3 deletions infer/src/pulse/PulseTaintItemMatcher.mli
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,11 @@ val get_tainted :
Tenv.t
-> PathContext.t
-> Location.t
-> TaintConfig.Unit.procedure_unit list
-> procedure_matchers:TaintConfig.Unit.procedure_unit list
-> field_matchers:TaintConfig.Unit.field_unit list
-> (Ident.t * Typ.t) option
-> has_added_return_param:bool
-> ?block_passed_to:Procname.t
-> Procname.t
-> TaintItem.value
-> BaseStack.value ProcnameDispatcher.Call.FuncArg.t list
-> AbductiveDomain.t
-> AbductiveDomain.t * (TaintItem.t * (BaseStack.value * Typ.t * Exp.t option)) list
69 changes: 48 additions & 21 deletions infer/src/pulse/PulseTaintOperations.ml
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ let taint_allocation tenv path location ~typ_desc ~alloc_desc ~allocator v astat
List.filter allocation_sources ~f:(fun (class_name, _) ->
String.equal class_name type_name )
in
(* Micro-optimisation: do not allocate `alloc_desc` when no matching taint sources are found *)
(* Micro-optimisation: do not allocate `alloc_desc` when no matching taint sources are found *)
if List.is_empty matching_allocations then None
else
let alloc_desc =
Expand Down Expand Up @@ -214,10 +214,10 @@ let taint_and_explore ~taint v astate =


let taint_sources tenv path location ~intra_procedural_only return ~has_added_return_param
?block_passed_to proc_name actuals astate =
potential_taint_value actuals astate =
let astate, tainted =
TaintItemMatcher.get_tainted tenv path location source_matchers return ~has_added_return_param
?block_passed_to proc_name actuals astate
TaintItemMatcher.get_tainted tenv path location ~procedure_matchers:source_matchers
~field_matchers:[] return ~has_added_return_param potential_taint_value actuals astate
in
List.fold tainted ~init:astate ~f:(fun astate (source, ((v, _), _, _)) ->
let hist =
Expand All @@ -243,10 +243,11 @@ let taint_sources tenv path location ~intra_procedural_only return ~has_added_re
astate ) ) )


let taint_sanitizers tenv path return ~has_added_return_param ~location proc_name actuals astate =
let taint_sanitizers tenv path return ~has_added_return_param ~location potential_taint_value
actuals astate =
let astate, tainted =
TaintItemMatcher.get_tainted tenv path location sanitizer_matchers return
~has_added_return_param proc_name actuals astate
TaintItemMatcher.get_tainted tenv path location ~procedure_matchers:sanitizer_matchers
~field_matchers:[] return ~has_added_return_param potential_taint_value actuals astate
in
let astate =
List.fold tainted ~init:astate ~f:(fun astate (sanitizer, ((v, history), _, _)) ->
Expand Down Expand Up @@ -409,17 +410,23 @@ let check_flows_wrt_sink ?(policy_violations_reported = IntSet.empty) path locat
((policy_violations_reported, astate), sanitizers) )


let should_ignore_all_flows_to proc_name =
Procname.is_objc_dealloc proc_name || BuiltinDecl.is_declared proc_name
let should_ignore_all_flows_to potential_taint_value =
match potential_taint_value with
| TaintItem.TaintProcedure proc_name ->
Procname.is_objc_dealloc proc_name || BuiltinDecl.is_declared proc_name
| _ ->
false


let taint_sinks tenv path location return ~has_added_return_param proc_name actuals astate =
let taint_sinks tenv path location return ~has_added_return_param potential_taint_value actuals
astate =
let astate, tainted =
TaintItemMatcher.get_tainted tenv path location sink_procedure_matchers return
~has_added_return_param proc_name actuals astate
TaintItemMatcher.get_tainted tenv path location ~procedure_matchers:sink_procedure_matchers
~field_matchers:sink_field_matchers return ~has_added_return_param potential_taint_value
actuals astate
in
PulseResult.list_fold tainted ~init:astate ~f:(fun astate (sink, ((v, history), _typ, _)) ->
if should_ignore_all_flows_to proc_name then Ok astate
if should_ignore_all_flows_to potential_taint_value then Ok astate
else
let sink_trace = Trace.Immediate {location; history} in
let visited = ref AbstractValue.Set.empty in
Expand Down Expand Up @@ -514,8 +521,9 @@ let propagate_to path location v values call astate =

let taint_propagators tenv path location return ~has_added_return_param proc_name actuals astate =
let astate, tainted =
TaintItemMatcher.get_tainted tenv path location propagator_matchers return
~has_added_return_param proc_name actuals astate
let potential_taint_value = TaintItem.TaintProcedure proc_name in
TaintItemMatcher.get_tainted tenv path location ~procedure_matchers:propagator_matchers
~field_matchers:[] return ~has_added_return_param potential_taint_value actuals astate
in
List.fold tainted ~init:astate ~f:(fun astate (_propagator, ((v, _history), _, _)) ->
let other_actuals =
Expand Down Expand Up @@ -691,6 +699,7 @@ let call tenv path location return ~call_was_unknown (call : _ Either.t) actuals
(SkippedUnknownCall call_exp) None actuals astate )
else Ok astate
| Second proc_name ->
let potential_taint_value = TaintItem.TaintProcedure proc_name in
let call_was_unknown = call_was_unknown || should_treat_as_unknown_for_taint tenv proc_name in
let has_added_return_param =
match IRAttributes.load proc_name with
Expand All @@ -700,16 +709,16 @@ let call tenv path location return ~call_was_unknown (call : _ Either.t) actuals
false
in
let astate =
taint_sanitizers tenv path (Some return) ~has_added_return_param ~location proc_name actuals
astate
taint_sanitizers tenv path (Some return) ~has_added_return_param ~location
potential_taint_value actuals astate
in
let astate =
taint_sources tenv path location ~intra_procedural_only:false (Some return)
~has_added_return_param proc_name actuals astate
~has_added_return_param potential_taint_value actuals astate
in
let+ astate =
taint_sinks tenv path location (Some return) ~has_added_return_param proc_name actuals
astate
taint_sinks tenv path location (Some return) ~has_added_return_param potential_taint_value
actuals astate
in
let astate, call_was_unknown =
let new_astate =
Expand All @@ -724,6 +733,17 @@ let call tenv path location return ~call_was_unknown (call : _ Either.t) actuals
else astate


let store tenv path location ~lhs:lhs_exp ~rhs:(rhs_exp, rhs_addr, typ) astate =
match lhs_exp with
| Exp.Lfield (_, field_name, _) ->
let potential_taint_value = TaintItem.TaintField field_name in
let rhs = {ProcnameDispatcher.Call.FuncArg.exp= rhs_exp; typ; arg_payload= rhs_addr} in
taint_sinks tenv path location None ~has_added_return_param:false potential_taint_value [rhs]
astate
| _ ->
Ok astate


let taint_initial tenv proc_name (proc_attrs : ProcAttributes.t) astate0 =
let result =
let++ astate, rev_actuals =
Expand All @@ -744,8 +764,15 @@ let taint_initial tenv proc_name (proc_attrs : ProcAttributes.t) astate0 =
~f:(fun ({passed_to} : ProcAttributes.block_as_arg_attributes) -> passed_to)
proc_attrs.ProcAttributes.block_as_arg_attributes
in
let potential_taint_value =
match block_passed_to with
| Some proc_name ->
TaintItem.TaintBlockPassedTo proc_name
| None ->
TaintItem.TaintProcedure proc_name
in
taint_sources tenv PathContext.initial proc_attrs.loc ~intra_procedural_only:true None
~has_added_return_param:false ?block_passed_to proc_name (List.rev rev_actuals) astate
~has_added_return_param:false potential_taint_value (List.rev rev_actuals) astate
in
match PulseOperationResult.sat_ok result with
| Some astate_tainted ->
Expand Down
9 changes: 9 additions & 0 deletions infer/src/pulse/PulseTaintOperations.mli
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,15 @@ val call :
-> AbductiveDomain.t AccessResult.t
(** add sources and sinks coming from a particular call site *)

val store :
Tenv.t
-> PathContext.t
-> Location.t
-> lhs:Exp.t
-> rhs:Exp.t * BaseStack.value * Typ.t
-> AbductiveDomain.t
-> AbductiveDomain.t AccessResult.t

val taint_allocation :
Tenv.t
-> PathContext.t
Expand Down
46 changes: 41 additions & 5 deletions infer/tests/codetoanalyze/objc/.infertaintconfig
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,20 @@
"sink_kinds": ["Logger"]
}
]
},
{ "short_description": "Token stored in ivar",
"taint_flows": [
{ "source_kinds": ["Token"],
"sink_kinds": ["StoreIvar"]
}
]
},
{ "short_description": "Token stored in ivar",
"taint_flows": [
{ "source_kinds": ["SimpleSource"],
"sink_kinds": ["StoreOtherIvar"]
}
]
}
],
"pulse-taint-sources": [
Expand All @@ -40,6 +54,16 @@
"class_names": ["AccessLibrary"],
"method_names": ["fetchResult"],
"kinds": ["SimpleSource"]
},
{
"block_passed_to": "Library.fetchWithCompletion:",
"taint_target": ["ArgumentPositions", [0]],
"kinds": ["Token"]
},
{
"class_names": ["OtherClass"],
"method_names": ["getSource"],
"kinds": ["SimpleSource"]
}
],
"pulse-taint-sanitizers": [
Expand Down Expand Up @@ -70,11 +94,23 @@
"taint_target": ["ArgumentPositions", [0]],
"kinds": ["SimpleSink"]
},
{ "class_names": ["Builder"],
"method_names": ["setValue:"],
"kinds": ["SimpleSink"],
"taint_target": ["ArgumentPositions", [1]]
}
{
"class_names": ["Builder"],
"method_names": ["setValue:"],
"kinds": ["SimpleSink"],
"taint_target": ["ArgumentPositions", [1]]
},
{
"field_regex": ".*",
"taint_target": "SetField",
"kinds": ["StoreIvar"]
},
{
"class_names": ["OtherClass"],
"field_names": ["_item"],
"taint_target": "SetField",
"kinds": ["StoreOtherIvar"]
}
],
"pulse-taint-propagators": [
{ "procedure": "URLCreate",
Expand Down
2 changes: 2 additions & 0 deletions infer/tests/codetoanalyze/objc/pulse/issues.exp
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,8 @@ codetoanalyze/objc/pulse/taint/InterProcTaintWithMock.m, taintInterprocWithMockB
codetoanalyze/objc/pulse/taint/NSMutableArrayTaint.m, add_object_propagator_bad, 7, TAINT_ERROR, no_bucket, ERROR, [source of the taint here: value returned from `SensitiveData.getSensitiveData` with kind `SimpleSource`,in call to `NSArray.objectEnumerator`,in call to function `NSEnumerator.nextObject` with no summary,in call to `NSMutableArray.addObject:`,value passed as argument `#0` to `my_log` with kind `SimpleSink`], source: SensitiveData.getSensitiveData, sink: my_log, tainted expression: targetAccounts
codetoanalyze/objc/pulse/taint/NilSourceTest.m, NilSourceTest.taintDataBad, 3, TAINT_ERROR, no_bucket, ERROR, [source of the taint here: value returned from `Data.data` with kind `SimpleSource`,value passed as argument `#0` to `taint_data` with kind `SimpleSink`], source: Data.data, sink: taint_data, tainted expression: item
codetoanalyze/objc/pulse/taint/SpecializedWithBlocksTaint.m, AccessLibrary.fetchWithCompletion:[specialized with blocks], 2, TAINT_ERROR, no_bucket, ERROR, [source of the taint here: value returned from `AccessLibrary.fetchResult` with kind `SimpleSource`,when calling `objc_block_fetchAndRunHandler_1[specialized with blocks]` here,when calling `objc_block_callSpecializedWithBlocksBad_2` here,value passed as argument `#1` to `Builder.setValue:` with kind `SimpleSink`], source: AccessLibrary.fetchResult, sink: Builder.setValue:, tainted expression: result
codetoanalyze/objc/pulse/taint/StoreIvarTaint.m, objc_block_StoreIvarTaint._fetchToken_bad_2, 4, TAINT_ERROR, no_bucket, ERROR, [source of the taint here: value passed as argument `#0` to a block passed to `Library.fetchWithCompletion:` with kind `Token`,in call to `NSArray.firstObject`,in call to function `Item.token` with no summary,in call to function `NSObject.copy` with no summary,when calling `StoreIvarTaint.setToken:` here,set `_token` with kind `StoreIvar`], source: a block passed to Library.fetchWithCompletion:, sink: _token, tainted expression: token
codetoanalyze/objc/pulse/taint/StoreIvarTaint.m, storeIvarBad, 3, TAINT_ERROR, no_bucket, ERROR, [source of the taint here: value returned from `OtherClass.getSource` with kind `SimpleSource`,when calling `OtherClass.setItem:` here,set `_item` with kind `StoreOtherIvar`], source: OtherClass.getSource, sink: _item, tainted expression: source
codetoanalyze/objc/pulse/taint/StringFormatTaint.m, stringPropagatesTaintBad, 4, TAINT_ERROR, no_bucket, ERROR, [source of the taint here: value returned from `NSURL.initWithString:` with kind `SensitiveURLSource`,in call to function `NSURL.scheme` with no summary,in call to `NSString.stringWithFormat:`,value passed as argument `#0` to `logEvent` with kind `Logger`], source: NSURL.initWithString:, sink: logEvent, tainted expression: urlInfo
codetoanalyze/objc/pulse/taint/URLPropagator.m, propagate_taint_url_bad1, 9, TAINT_ERROR, no_bucket, ERROR, [source of the taint here: value returned from `NSURL.initWithString:` with kind `SensitiveURLSource`,in call to `URLCreate`,value passed as argument `#0` to `logEvent` with kind `Logger`], source: NSURL.initWithString:, sink: logEvent, tainted expression: url
codetoanalyze/objc/pulse/taint/URLPropagator.m, propagate_taint_url_bad2, 9, TAINT_ERROR, no_bucket, ERROR, [source of the taint here: value returned from `NSURL.initWithString:` with kind `SensitiveURLSource`,in call to `URLCreate1`,value passed as argument `#0` to `logEvent` with kind `Logger`], source: NSURL.initWithString:, sink: logEvent, tainted expression: url
Expand Down
88 changes: 88 additions & 0 deletions infer/tests/codetoanalyze/objc/pulse/taint/StoreIvarTaint.m
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/*
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

#import <Foundation/Foundation.h>

@interface Item : NSObject
@property(nonatomic, copy) NSString* token;
@end

typedef void (^LibraryFetchCompletionBlock)(NSArray<id>* items,
NSArray<NSError*>* errors);

@interface Library : NSObject
+ (instancetype)sharedInstance;
@end

@implementation Library

+ (instancetype)sharedInstance {
static dispatch_once_t dispatchOnceToken;
static Library* sharedLibrary = nil;
dispatch_once(&dispatchOnceToken, ^{
sharedLibrary = [Library new];
});
return sharedLibrary;
}

- (void)fetchWithCompletion:(LibraryFetchCompletionBlock)completion {
}

@end

@interface StoreIvarTaint : NSObject

@end

@implementation StoreIvarTaint {
NSString* _token;
}

- (void)_fetchToken_bad {
__weak __typeof(self) weakSelf = self;
[[Library sharedInstance]
fetchWithCompletion:^(NSArray<Item*>* accounts,
NSArray<NSError*>* _Nonnull errors) {
NSString* const accessToken = [accounts firstObject].token;
NSString* token = [accessToken copy];
[self setToken:token];
}];
}

- (void)setToken:(NSString*)token {
_token = token;
}

@end

@interface OtherClass : NSObject
- (NSString*)getSource;
@end

@implementation OtherClass {
NSString* _item;
@public
NSString* _name;
}

- (void)setItem:(NSString*)item {
_item = item;
}

@end

void storeIvarBad() {
OtherClass* c = [OtherClass new];
NSString* source = [c getSource];
[c setItem:source];
}

void storeIvarGood() {
OtherClass* c = [OtherClass new];
NSString* source = [c getSource];
c->_name = source;
}

0 comments on commit 109e7a1

Please sign in to comment.