A fix from a security static analysis #202
Conversation
…checked for returning NO before using the result.
| @@ -843,7 +843,8 @@ - (id)init | |||
| app = (char *)malloc(appLen + 1); | |||
| if (app == NULL) return nil; | |||
|
|
|||
| [appName getCString:app maxLength:(appLen+1) encoding:NSUTF8StringEncoding]; | |||
| BOOL processedAppName = [appName getCString:app maxLength:(appLen+1) encoding:NSUTF8StringEncoding]; | |||
| if (NO == processedAppName) return nil; | |||
There was a problem hiding this comment.
So if we can't get an appName initialization is just aborted?
There was a problem hiding this comment.
That would be an effect of this change, as currently happens if we can't get a PID.
We should do something if processedAppName is NO, as that means that app could contain invalid data; another option would b e to set app to "".
There was a problem hiding this comment.
Or something like <UnnamedApp>.
Probably it would be even better to investigate the cases where appName can't be processed.
What do you think @dvor?
There was a problem hiding this comment.
<UnnamedApp> or <Unknown> sounds good for any of these error cases.
Note that this 'bug' was found via a securiy-oriented static analysis; I do not have a real world test case, but it is theoretically possible for getCString:maxLength:encoding: to return NO if the string isn't actually valid UTF8 or if the buffer were too small (which shouldn't happen since we just allocated it to the appropriate size).
There was a problem hiding this comment.
User can set processName:
[[NSProcessInfo processInfo] setProcessName:<some not valid string>];Though it's pretty rare case, it would be better to use <UnnamedApp> instead of aborting initialization. @evands would you make a change in this pull request?
There was a problem hiding this comment.
I think the cleanest thing to do is to check the return of lengthOfBytesUsingEncoding, as the documentation states:
Returns 0 if the specified encoding cannot be used to convert the receiver or if the amount of memory required for storing the results of the encoding conversion would exceed NSIntegerMax
we can in that case reassign appName to <UnnamedApp>.
If we have done that, then we get to the cString conversion and it still fails, aborting the initialization makes sense as something is thoroughly wrong.
|
@rivera-ernesto could you check this out? It looks fine to me, but I'd like you to see it before getting it merged. |
|
Looks good, I think we can merge it. |
|
@rivera-ernesto @dvor should we release after this pull request is merged? I suggest 1.8.0 (1.7.1 would mean we have just a patch and we did add CLI support). |
|
I think that we can implement ability to set custom log filename before releasing (discussion in #207). I'll try to deal with it today or tomorrow. |
|
Cool. Can I help with that? |
|
Well, I've already dived into |
|
👍 |
A fix from a security static analysis
Fix the remaining case in which getCString:maxLength:encoding is not checked for returning NO before using the result.