WIP: OSCMD2:Add oscmdpatches report for Command Injection analysis#280
Conversation
dvorak42
changed the title
waskyo
left a comment
waskyo
left a comment
There was a problem hiding this comment.
some questions and minor comments!
| buffersize, sizeorigin = calculate_buffer_size(stackframe, argoffset, instr) | ||
| fn_args.append({"type": "pointer", "max-length": buffersize, "size-origin": sizeorigin}) | ||
| else: | ||
| fn_args.append({"type": "unknown"}) |
There was a problem hiding this comment.
i think we should log a warning here? (which would normally cause the rest of the pipeline to fail)
There was a problem hiding this comment.
We do want to be able to continue in this case, if we're able to derive the type from the format string (in hawkeye), we can replace this type with "string" or "int". Some of the doSystemCmd cases have some non-stack address passed in.
Added a comment.
There was a problem hiding this comment.
I agree with Ricardo that we probably want a warning here, because it is extremely rare for a format argument to actually be a pointer.
I have just merged in code that exposes the format string specifiers produced by the ocaml analyzer. The code is in chb/api/FormatStringSpec.py, A particular format string spec is accessed from the Function object with the address of the instruction that has the format string as argument: function.formatstrings[iaddr].
Could you please integrate this code (and expand FormatStringSpec.py as necessary to return the types instead of just the specifiers)?
If this would take care of most of the cases not handled right now, I think it's worth to add the warning.
There was a problem hiding this comment.
Are there additional parameters to pass into codehawk? It looks like the only format string this recognizes in insulin_inject is the "exection failed" one, and missing the one with actual strings (and its not finding any of the strings passed into doSystemCmd in the other case).
There was a problem hiding this comment.
It shouldn't need any additional parameters, but it does require that the analysis knows about the format string, either from a function summary, or from an annotated signature in a header file, or from an inferred function summary. It doesn't discover format strings, it just records them.
There was a problem hiding this comment.
Shouldn't it infer the format string for both snprintf calls in insulin_inject (function 0x119a0, it finds the one at 0x11ba8 but not the one at 0x11aa0)?
Looking at the function analysism, I think it knows about the "/sbin/ifconfig %s 2>&1" format string at 0x11aa0 (there's a po for 0x11aa0: output-format-string("/sbin/ifconfig %s 2>&1") (safe, constant string: /sbin/ifconfig %s 2>&1), but it doesn't seem to generate the format string specifier?
There was a problem hiding this comment.
I think the problem with the recorded format strings was that they were not persistent: only the ones recorded in the last analysis round were kept. I have checked in a new version (codehawk/CodeHawk-Binary) to make them persistent. Could you please try again? Thank you.
There was a problem hiding this comment.
Yup, that seems to fix it, and the doSystemCmd ones also show up now. Thanks, I'll update the CL to use those now.
There was a problem hiding this comment.
The latest patch set should incorporate this change and should be ready for another review.
waskyo
left a comment
waskyo
left a comment
There was a problem hiding this comment.
This looks good to me, but I don't know enough about CHB to faithfully say "ship it". Maybe Henny can review too?
Thank you!
dvorak42
force-pushed
the
dvorak42/add_oscmdpatches
branch
from
bb43d7d to
1b5302a
Compare
|
@sipma Could you review this change to add oscmdpatches to Codehawk-Binary. |
| buffersize, sizeorigin = calculate_buffer_size(stackframe, argoffset, instr) | ||
| fn_args.append({"type": "pointer", "max-length": buffersize, "size-origin": sizeorigin}) | ||
| else: | ||
| fn_args.append({"type": "unknown"}) |
There was a problem hiding this comment.
I agree with Ricardo that we probably want a warning here, because it is extremely rare for a format argument to actually be a pointer.
I have just merged in code that exposes the format string specifiers produced by the ocaml analyzer. The code is in chb/api/FormatStringSpec.py, A particular format string spec is accessed from the Function object with the address of the instruction that has the format string as argument: function.formatstrings[iaddr].
Could you please integrate this code (and expand FormatStringSpec.py as necessary to return the types instead of just the specifiers)?
If this would take care of most of the cases not handled right now, I think it's worth to add the warning.
…injection patches.
dvorak42
force-pushed
the
dvorak42/add_oscmdpatches
branch
from
1b5302a to
6032402
Compare
sipma
left a comment
sipma
left a comment
There was a problem hiding this comment.
Looks good. Thank you for doing the update.
3 participants
No description provided.