Veracode CWE ID 416: Use After Free

Question

How to fix Veracode Use After Free (CWE ID 416)

Recommendations from Veracode: Ensure that all pointers are set to NULL once the memory they point to has been freed.

Error pointed on: Line 8 "return result;"

+ (NSData *)dataFromBase64String:(NSString *)aString
{
    NSData *data = [aString dataUsingEncoding:NSASCIIStringEncoding];
    size_t outputLength = 0;
    void *outputBuffer = NewBase64Decode([data bytes], [data length], &outputLength);
    NSData *result = [NSData dataWithBytes:outputBuffer length:outputLength];
    free(outputBuffer);
    return result;
}

@OlSen You mean to say if (outputBuffer != NULL) { free(outputBuffer); } ? But they have pointed error on "return result;" — Prajnaranjan Das, Sep 09 '21 at 18:31
Your interpretation would also work. Because if it outputBuffer is not NULL it needs to be free again. The result must return, you cant free it before u return but you could return nil if ouputBuffer==NULL — Ol Sen, Sep 09 '21 at 19:01
Do you have any reason to believe this tool knows what it's talking about (particularly in terms of what line it points to?) From your questions on this tool, it sounds like the correct approach is to open support tickets with Veracode. I suspect their tool is broken. — Rob Napier, Sep 14 '21 at 19:20
@OlSen I tried the above solution you had mentioned but nothing passed in veracode's scan report. Can you please guide me how to solve it now ? — Prajnaranjan Das, Sep 15 '21 at 19:10

Ol Sen · Answer 1 · 2021-09-16T14:51:39.393

Veracode scan does just help you to find places in code where you could improve security related coding. It does not stop attacks of course, but if your application is really that much security oriented you can make it more difficult to read out memory that is left after processing.

The word "Error" in the logging of Veracode is maybe a bit overused..
But my suggestion to address Veracodes Error pointed on: Line 8 "return result;" would be..

+ (NSData *)dataFromBase64String:(NSString *)aString
{
    if (aString!=nil && [aString length]) {
        size_t outputLength = 0;
        void *outputBuffer = NULL;
        NSData *data = [aString dataUsingEncoding:NSASCIIStringEncoding];
        outputBuffer = NewBase64Decode([data bytes], [data length], &outputLength);
        if (outputBuffer==NULL) return nil; //if NewBase64Decode() failed there is nothing to free..
        NSData *result = [NSData dataWithBytes:outputBuffer length:outputLength];
        free(outputBuffer);
        outputBuffer = NULL;
        return result;
    }
    return nil;
}

This is because free'd memory is not set to NULL without your intent, so someone scanning memory for left overs would maybe find some clues about the former content of address.

here some nice discussion if NULL after free is really needed.

If you go that much into detail to avoid any kind of risk then you could also initiate outputBuffer with NULL (void* outputBuffer = NULL;) before you even use it.

Well it is another discussion if this is a bit overdo for some objC code where just swizzling could override the whole method.

EDIT: even more spagetti code, trying to avoid returning any value other than void and change a passed argument instead.

+ (void)dataFromBase64String:(NSString *)aString toResult:(NSData**)result
{
    if (aString!=nil && [aString length]) {
        size_t outputLength = 0;
        void *outputBuffer = NULL;
        NSData *data = [aString dataUsingEncoding:NSASCIIStringEncoding];
        outputBuffer = NewBase64Decode([data bytes], [data length], &outputLength);
        if (outputBuffer==NULL) return; //if NewBase64Decode() failed there is nothing to do
        *result = [NSData dataWithBytes:outputBuffer length:outputLength];
        free(outputBuffer);
        outputBuffer = NULL;
    }
}

//and call like..
NSData *myresult = nil;
[YOURCLASS dataFromBase64String:@"someString" toResult:&myresult];
NSLog(@"result=%@",myresult);

Now i wonder what Veracode is reporting with the edit above..

Thanks for this answer. I tried the same answer and rescanned my code with Veracode but still their result is same. They are pointing to line number 13 "}" in the above code which you mentioned. Can you please suggest what we are doing wrong here ? — Prajnaranjan Das, Sep 16 '21 at 09:06
Thats the thing @RobNapier pointed out. Reporting a basic return as "error" that is just triggering the release of used stack after leaving the methods block is what i would call misleading overdo of Veracode. I would suggest to ignore that "error". But it should have reported line 8 too. It may be that the return of NSData at all is seen as risky, then you could return void and instead directly change a passed argument, just like it is done in NewBase64Decode for outputLength.. — Ol Sen, Sep 16 '21 at 14:35

Veracode CWE ID 416: Use After Free

1 Answers1