Problem: RestAPI calls are not auditable#661
Conversation
| ZmsgGuard msg (mlm_client_recv (client)); | ||
| ZstrGuard status (zmsg_popstr (msg)); | ||
| ZstrGuard uuid (zmsg_popstr (msg)); | ||
| if (!streq (status, "OK")) { |
There was a problem hiding this comment.
Not sure that streq() support NULL arg. + ZstrGuard() usage. Be carefull here.
There was a problem hiding this comment.
I don't know as well, but that's what's been used in the system, this is no new code.. As per first comment, ignore whitespace changes to see added code. But for sure we can discuss fixing things in rest here, but it's not aim of this PR
There was a problem hiding this comment.
Note that effectively Jeff's question goes for implementation of strcmp() because in ZMQ stack we #define streq(s1,s2) (!strcmp ((s1), (s2))). The standard does not define strcmp behavior for NULL inputs, so anything is allowed to happen, from a sanity check to a segmentation fault, any answer is correct.
There was a problem hiding this comment.
Looking at czmq code at https://github.com/42ity/czmq/blob/v3.0.2-FTY-master/src/zmsg.c#L407 and https://github.com/42ity/czmq/blob/v3.0.2-FTY-master/src/zframe.c#L238 there are several scenarios that zmsg_popstr() would return NULL, and we do not check nor at least assert for such cases anywhere, so Jeff's concern is valid :)
There was a problem hiding this comment.
All that said, if stars align and reading (dereferencing) data from NULL pointer reads some garbage (or the OS-mapped zero-filled page for example), so the program does not crash on this read, the strcmp() would quickly and highly-probably find there is no OK in the status so decide to ... (whatever it needs the clause for).
There was a problem hiding this comment.
While the question is right, this is a wrong place for such a question. I think this should be promoted to a task to check RestAPI for such uses of possibly invalid recvd data
There was a problem hiding this comment.
Sounds correct: the goal of this PR is different.
|
We use log_info_audit() to log both successes and failures |
|
For sure we can use log_audit_error, I just don't know whether that's our aim as this is different log level. |
jimklimov
left a comment
jimklimov
left a comment
There was a problem hiding this comment.
Looks practically good, with one C scope-change comment to discuss and maybe address or not as a result.
|
|
||
| // sanity check | ||
| std::string id = request.getArg ("id"); | ||
| { |
There was a problem hiding this comment.
Why this brace (and scoping) move? I see we log id below, but could actually log the checked_id instead as it is the value we use and trust (or bail out in this clause). The idea for checked* stuff was that the original untrusted input variables do not leak around (checked ones might be sanitized, etc.).
There was a problem hiding this comment.
Also it seems that id is logged in many other files, where this apparently does not cause scoping issues... maybe it should have ;)
There was a problem hiding this comment.
Scope is still local file, just a bit extended to be able to log it. While I agree we can use this variable in the block, and then use checked one, the result should still be the same, right?
There was a problem hiding this comment.
Should be... but only as long as the checked_* values we use are a guaranteed copy, not a conversion product, of the input * query arguments.
|
merging to test, will revert in case of issues |
| std::string id = request.getArg ("id"); | ||
|
|
||
| if (id.empty () || !persist::is_ok_name (id.c_str ())) | ||
| log_info_audit ("Request PUT asset FAILED"); |
There was a problem hiding this comment.
Oopsie! No parenthesis, no luck!
3 participants
Solution: add audit logging calls to show what user did (or didn't do)
Note: for review ignore whitespace changes