PR#46: fix possible double free in `klist -A` - rpms/krb5

rpms / krb5

#46 fix possible double free in `klist -A`

Closed 4 months ago by jrische. Opened 4 months ago by dtardon.

rpms/ dtardon/krb5 klist-double-free into rawhide

fix possible double free in `klist -A`

David Tardon • 4 months ago

91fb9a1

0017-set-defname-to-NULL-after-freeing-it.patch

file added

+34

		`@@ -0,0 +1,34 @@`
		`+ From d9686b9da8ae20a771afa59984a6fb4d4462e120 Mon Sep 17 00:00:00 2001`
		`+ From: rpm-build <rpm-build>`
		`+ Date: Mon, 8 Jan 2024 10:41:50 +0100`
		`+ Subject: [PATCH] set defname to NULL after freeing it`
		`+`
		`+ It's a static variable, hence it will be reused on next call of the`
		`+ function, leading to double free if krb5_cc_get_principal() fails:`
		`+`
		`+ klist: Credentials cache keyring 'persistent:1000:krb_ccache_xgrbUbg' not found`
		`+`
		`+ klist: Credentials cache keyring 'persistent:1000:krb_ccache_1jASmDA' not found`
		`+ free(): double free detected in tcache 2`
		`+ Aborted (core dumped)`
		`+`
		`+ This is a regression from 0016-Fix-unimportant-memory-leaks.patch .`
		`+ ---`
		`+ src/clients/klist/klist.c \| 1 +`
		`+ 1 file changed, 1 insertion(+)`
		`+`
		`+ diff --git a/src/clients/klist/klist.c b/src/clients/klist/klist.c`
		`+ index 43392d2..6c43c0f 100644`
		`+ --- a/src/clients/klist/klist.c`
		`+ +++ b/src/clients/klist/klist.c`
		`+ @@ -525,6 +525,7 @@ cleanup:`
		`+ (void)krb5_cc_end_seq_get(context, cache, &cur);`
		`+ krb5_free_principal(context, princ);`
		`+ krb5_free_unparsed_name(context, defname);`
		`+ + defname = NULL;`
		`+ return status;`
		`+ }`
		`+`
		`+ --`
		`+ 2.43.0`
		`+`

krb5.spec

file modified

		`@@ -75,6 +75,7 @@`
		`Patch0014: 0014-Enable-PKINIT-if-at-least-one-group-is-available.patch`
		`Patch0015: 0015-Replace-ssl.wrap_socket-for-tests.patch`
		`Patch0016: 0016-Fix-unimportant-memory-leaks.patch`
		`+ Patch0017: 0017-set-defname-to-NULL-after-freeing-it.patch`

		`License: BSD-2-Clause AND (BSD-2-Clause OR GPL-2.0-or-later) AND BSD-3-Clause AND BSD-4-Clause AND FSFULLRWD AND HPND-export-US AND HPND-export-US-modify AND ISC AND MIT AND MIT-CMU AND OLDAP-2.8 AND RSA-MD`
		`URL: https://web.mit.edu/kerberos/www/`

no initial comment

Metadata Update from @jrische:
- Request assigned

4 months ago

jrische commented 4 months ago

Hello David,

Thank you very much for reporting this bug. Given the amount of changes in the recent memory leaks patch, I was worried issues like this one would occur.

The problem is actually worst that what you are describing in the message of your patch: The "defname" variable is actually a global one, not a static one, but most importantly it is not initialized, which is a problem especially like in you case when the call to krb5_cc_get_principal() fails, and the execution jumps to the "cleanup" section where "defname" is freed. This means that if the retrieval of the principal from the ccache fails, then the program will try to free an arbitrary memory address, since nothing was assigned to "defname" so far.

I don't think "defname" should be a global variable at all. It should be a local variable initialized with NULL at the beginning of show_ccache(). I suspect if was done this way because "defname" is used in show_credential(), which is called in show_ccache() is itself. I suppose this was done to access the string format of the principal without having to pass it as a proper parameter to show_credential().

I think this issue should be fixed upstream by turning "defname" into a local variable and passed to show_credential() as a proper parameter. I will open an upstream pull request and backport here as soon as possible.

jrische commented 4 months ago

My bad, actually in C89 a global variable is implicitly initialized to 0 if it has no initializer:

If an object that has static storage duration is not initialized explicitly, it is initialized implicitly as if every member that has arithmetic type were assigned 0 [...]

http://port70.net/~nsz/c/c89/c89-draft.html#3.5.7

So as you wrote in your patch, the only failure to expect is the double free. Still I think it would be safer to turn "defname" into a local variable.

Pull-Request has been closed by jrische

4 months ago

jrische commented 4 months ago

Closed in favor of #47 and #48.