Cwe476 #11
Cwe476 #11
Conversation
| type t = (Var.t * Tid.t) list | ||
|
|
||
| (** adds var as a tainted register (with the taint source given by tid) *) | ||
| let add state var tid = |
There was a problem hiding this comment.
Even though you are using the power of scopes, it would be clearer to write "old_state" and "new_state" instead of using just "state".
There was a problem hiding this comment.
I am not sure how to introduce "old_state" and "new_state" without decreasing readability. I could rename State.add to something like State.add_elem, but you probably had something else in mind.
There was a problem hiding this comment.
I am referring to the name "state" that you use for several variables (e.g. as parameter for the fun, in the let clause)
There was a problem hiding this comment.
I could rename state to something like tainted_register_list so that its purpose gets more obvious. I just do not think that I should name something "new state", because it would always be the return value and naming return values in ocaml feels unnecessary verbose.
src/checkers/cwe_476.ml
Outdated
| end | ||
| | _ -> state | ||
|
|
||
| type cwe_hits_type = Tid.t list ref |
There was a problem hiding this comment.
I am still not sure where to declare such global variables: locality vs. header of file.
There was a problem hiding this comment.
In this case I should simply delete the type definition. I just realized that I only use it once.
There was a problem hiding this comment.
Well, the definition is not useless, I was just wondering about the right place to declare it. That is more a question of style, I suppose.
| let print_hit tid ~sub ~function_names ~tid_map = | ||
| let block = Option.value_exn (Term.find blk_t sub tid) in | ||
| let jmps = Term.enum jmp_t block in | ||
| let _ = Seq.find_exn jmps ~f:(fun jmp -> |
There was a problem hiding this comment.
this let _ = something in () seems to be strange. Couldn't you use Seq.iter, which also returns union?
There was a problem hiding this comment.
Seq.find_exn seems the most logical choice for me, because it is supposed to find exactly one element and it throws an exception if it doesn't find anything. Usually I would use the return value for the Log_utils.warn call, but then I would have to unpack the jmp again.
|
|
||
| (** Updates the state depending on the jump. On a jump to a function from the function list | ||
| taint all return registers as unchecked return values. *) | ||
| let update_state_jmp jmp state ~cwe_hits ~function_names ~program ~block ~strict_call_policy = |
There was a problem hiding this comment.
this function is rather long, can't you refactor it and break it down into smaller units?
There was a problem hiding this comment.
The function is a little bit shorter now.
src/checkers/cwe_476.ml
Outdated
| ) in () | ||
|
|
||
| let check_cwe prog proj tid_map symbol_pairs = | ||
| let strict_call_policy = true in |
There was a problem hiding this comment.
make this configurable in JSON config since it might be a good way to tune false positives/false negatives. Document this parameter accordingly.
There was a problem hiding this comment.
I've added a general way to add parameters to the checks configurable in config.json.
src/checkers/cwe_476.ml
Outdated
| let block = Graphs.Ir.Node.label node in | ||
| update_block_analysis block state ~cwe_hits ~function_names ~program:prog ~strict_call_policy | ||
| ) in | ||
| let _ = Graphlib.Std.Graphlib.fixpoint (module Graphs.Ir) cfg ~steps:100 ~rev:false ~init:init ~equal:equal ~merge:merge ~f:f in |
There was a problem hiding this comment.
100 is a magic number, should it be configurable? Should we set to some lower threshold?
There was a problem hiding this comment.
I made it configurable. But it is still a magic number. I suppose that 20 could also be enough, but higher numbers are safer to prevent false negatives.
Oh and it does not cost computing time in the absence of possible cwe-hits.
No description provided.