View Issue Details

IDProjectCategoryView StatusLast Update
0005078unrealircdpublic2018-04-22 09:39
ReporterSky-DancerAssigned Tosyzop 
PrioritylowSeverityminorReproducibilitysometimes
Status resolvedResolutionfixed 
Product Version4.0.17 
Target VersionFixed in Version4.0.18 
Summary0005078: ssl pointer not freed on error
Descriptionin function;
int cipher_check(SSL_CTX *ctx, char **errstr) => ssl.c file, line 971
if there is a "weak" cipher or "NULL" cipher; in while loop, temp ssl pointer not freed on zero return.
Steps To Reproduceset weak cipher to cipher list.
Additional InformationNot a big problem but this is a little memory leak on weak cipher check's exit.
TagsNo tags attached.
3rd party modules

Activities

syzop

2018-03-25 12:59

administrator   ~0020067

Last edited: 2018-03-25 13:01

View 2 revisions

Hi,

Thanks for the report. Not many people reading the UnrealIRCd source code nowadays :)

In this case there's no bug. Let me explain:

int verify_certificate(SSL *ssl, char *hostname, char **errstr)
{
    static char buf[512];

This means the 'buf' is a static buffer that can safely be returned and/or used outside this function. It's a bit comparable to a global variable of char buf[512].

So it can safely set or return it, which is what happens here:

    if (errstr)
        *errstr = buf;

But in other functions that use such static char [] you could see something like:

    return buf;

This is no problem. The data in 'buf' can safely be used outside the function. The only downside is that once verify_certificate() is called again, it will overwrite the contents of 'buf'.
It's a common way in C to do things. It saves the hassle of allocating and freeing memory on the heap, which is often 1) forgotten (memory leak), or 2) done twice (double free).
Of course, as I said, the next time the function is called the 'buf' is overwritten, so this method can only be used when the caller immediately uses the value.

I'm sure there are resources on the internet out there that explain this better than me ;)

Sky-Dancer

2018-03-27 03:02

reporter   ~0020074

i mean the "ssl" named pointer.
you made "ssl = SSL_new(ctx);" (line 984)
after that;
if there is no error, "SSL_free(ssl);" will be execute, and this is ok. (line 1017)
but, if cipher str contains "DES-" or "3DES-" or "RC4-" or "NULL-" you didn't free the ssl named pointer. (lines; 998 1003 1008 1013)

codes will be like this i think;

***
        if (strstr(cipher, "DES-"))
        {
            snprintf(errbuf, sizeof(errbuf), "DES is enabled but is a weak cipher");
            SSL_free(ssl); // ?
            return 0;
        }
        else if (strstr(cipher, "3DES-"))
        {
            snprintf(errbuf, sizeof(errbuf), "3DES is enabled but is a weak cipher");
            SSL_free(ssl); // ?
            return 0;
        }

***
PS. : i know there will be ONLY one address of static char buffer in c :P

syzop

2018-03-27 08:04

administrator   ~0020075

Ah, 'ssl'. Right, ok.

syzop

2018-04-22 09:39

administrator   ~0020103

Fixed, thanks for the report :)

https://github.com/unrealircd/unrealircd/commit/a7bcb637b731cc3e3791f573b0b9988fa3b4ba41

commit a7bcb637b731cc3e3791f573b0b9988fa3b4ba41
Author: Bram Matthys <syzop@vulnscan.org>
Date: Sun Apr 22 09:37:06 2018 +0200

    Fix small memory leak if not passing the weak cipher config test (DES/3DES..)
    Reported by Sky-Dancer (0005078).

Issue History

Date Modified Username Field Change
2018-03-16 17:33 Sky-Dancer New Issue
2018-03-25 12:59 syzop Note Added: 0020067
2018-03-25 13:01 syzop Note Edited: 0020067 View Revisions
2018-03-27 03:02 Sky-Dancer Note Added: 0020074
2018-03-27 08:04 syzop Note Added: 0020075
2018-03-27 08:04 syzop Status new => acknowledged
2018-04-22 09:39 syzop Assigned To => syzop
2018-04-22 09:39 syzop Status acknowledged => resolved
2018-04-22 09:39 syzop Resolution open => fixed
2018-04-22 09:39 syzop Fixed in Version => 4.0.18
2018-04-22 09:39 syzop Note Added: 0020103