View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0005078 | unreal | ircd | public | 2018-03-16 17:33 | 2018-04-22 09:39 |
| Reporter | Sky-Dancer | Assigned To | syzop | ||
| Priority | low | Severity | minor | Reproducibility | sometimes |
| Status | resolved | Resolution | fixed | ||
| Product Version | 4.0.17 | ||||
| Fixed in Version | 4.0.18 | ||||
| Summary | 0005078: ssl pointer not freed on error | ||||
| Description | in 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 Reproduce | set weak cipher to cipher list. | ||||
| Additional Information | Not a big problem but this is a little memory leak on weak cipher check's exit. | ||||
| Tags | No tags attached. | ||||
| 3rd party modules | |||||
|
|
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 ;) |
|
|
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 |
|
|
Ah, 'ssl'. Right, ok. |
|
|
Fixed, thanks for the report :) https://github.com/unrealircd/unrealircd/commit/a7bcb637b731cc3e3791f573b0b9988fa3b4ba41 commit a7bcb637b731cc3e3791f573b0b9988fa3b4ba41 Author: Bram Matthys <[email protected]> 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). |
| 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 | |
| 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 |
