Always clear username/password from memory on error

PATCH Superseded Steffan Karger 4 participants 7 messages May 9, 2017

lore

Share & tools

lore

Steffan Karger

author

2017-05-09 10:50 UTC #0 lore raw

PATCH 1/1 Always clear username/password from memory on error

This issue was found by Quarkslab during the OSTIF-founded security audit (issue 5.4), we agree with their analysis:

"There’s a special case where the client username and password are not erased when the server is launched without an external script or authentication plugin. While being invalid, this configuration does not raise any error. If the client transmits its credentials and the session is not established (for instance if the certificates chain has not been verified), these credentials are not erased from memory by the server.

The likelihood of an occurrence of this issue in real life is exceptionally low since an attacker needs elevated privileges on the server to exploit this kind of information leak. The severity of this issue is rated as very low."

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/ssl.c | 1 +
 1 file changed, 1 insertion(+)

src/openvpn/ssl.c +1 -0

index 1033e58..abf5786 100644
@@ -2661,6 +2661,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio
 
 error:
     secure_memzero(ks->key_src, sizeof(*ks->key_src));
+    secure_memzero(up, sizeof(*up));
     buf_clear(buf);
     gc_free(&gc);
     return false;

Signature · 1 line

2.7.4

David Sommerseth

2017-05-09 17:47 UTC #1 lore raw

Quoted 26 lines · Steffan Karger

This issue was found by Quarkslab during the OSTIF-founded security audit (issue 5.4), we agree with their analysis:
"There’s a special case where the client username and password are not erased when the server is launched without an external script or authentication plugin. While being invalid, this configuration does not raise any error. If the client transmits its credentials and the session is not established (for instance if the certificates chain has not been verified), these credentials are not erased from memory by the server.
The likelihood of an occurrence of this issue in real life is exceptionally low since an attacker needs elevated privileges on the server to exploit this kind of information leak. The severity of this issue is rated as very low."
Signed-off-by Steffan Karger <[email protected]...>
---
 src/openvpn/ssl.c | 1 +
 1 file changed, 1 insertion(+)
src/openvpn/ssl.c +0 -0
index 1033e58..abf5786 100644

src/openvpn/ssl.c key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio · line 2667

Quoted 7 lines

src/openvpn/ssl.c +1 -0

@@ -2661,6 +2661,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio
 
 error:
     secure_memzero(ks->key_src, sizeof(*ks->key_src));
+    secure_memzero(up, sizeof(*up));
     buf_clear(buf);
     gc_free(&gc);
     return false;

Feature-ACK, but code-NAK.

In key_method_2_read() there are a few places where goto error; can be used before up is allocated. This will make secure_memzero() segfault, as it does not handle NULL pointers. Or even worse (if the compiler let this pointer point at a random address), which leads to an unpredictable behaviour.

That said, I think we should fix secure_memzero() to just return if the input pointer is NULL. And even though most compilers do initialize variables, I think it's good to be defensive here and initialize up too.

Signature · 4 lines

kind regards,

David Sommerseth
OpenVPN Technologies, Inc

Selva Nair

2017-05-09 18:08 UTC #2 lore raw

Hi,

Quoted 4 lines · David Sommerseth

That said, I think we should fix secure_memzero() to just return if the input pointer is NULL. And even though most compilers do initialize variables, I think it's good to be defensive here and initialize up too.

No, compiler will not initialize such non-static local variables. In fact in this case gcc will warn (with -Wall) that "up" may be used uninitalized because of code paths where "up" is not allocated as you pointed out.

In that sense its better not to initialize such variables as initialization can hide errors.

Selva

David Sommerseth

2017-05-09 18:23 UTC #3 lore raw

Quoted 12 lines · Selva Nair

Hi,
On Tue, May 9, 2017 at 1:47 PM, David Sommerseth
<[email protected]...
<mailto:[email protected]...>> wrote:
That said, I think we should fix secure_memzero() to just return if the input pointer is NULL. And even though most compilers do initialize variables, I think it's good to be defensive here and initialize up too.

Quoted 7 lines

No, compiler will not initialize such non-static local variables. In fact in this case gcc will warn (with -Wall) that "up" may be used uninitalized because of code paths where "up" is not allocated as you pointed out.
In that sense its better not to initialize such variables as initialization can hide errors.

I agree that getting the warning is useful for developers. But I've just seen way too many packagers ignoring these warnings (or even removing -Wall) - and then ship binaries with undefined behaviour. I don't like that scenario at all.

Signature · 4 lines

kind regards,

David Sommerseth
OpenVPN Technologies, Inc

Steffan Karger

author

2017-05-09 18:28 UTC #4 lore raw

In reply to David Sommerseth

Quoted 43 lines · David Sommerseth

Quoted 26 lines · Steffan Karger
This issue was found by Quarkslab during the OSTIF-founded security audit (issue 5.4), we agree with their analysis:
"There’s a special case where the client username and password are not erased when the server is launched without an external script or authentication plugin. While being invalid, this configuration does not raise any error. If the client transmits its credentials and the session is not established (for instance if the certificates chain has not been verified), these credentials are not erased from memory by the server.
The likelihood of an occurrence of this issue in real life is exceptionally low since an attacker needs elevated privileges on the server to exploit this kind of information leak. The severity of this issue is rated as very low."
Signed-off-by Steffan Karger <[email protected]...>
---
 src/openvpn/ssl.c | 1 +
 1 file changed, 1 insertion(+)
src/openvpn/ssl.c +0 -0
index 1033e58..abf5786 100644
src/openvpn/ssl.c key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio · line 2667
Quoted 7 lines
src/openvpn/ssl.c +1 -0
@@ -2661,6 +2661,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio
 
 error:
     secure_memzero(ks->key_src, sizeof(*ks->key_src));
+    secure_memzero(up, sizeof(*up));
     buf_clear(buf);
     gc_free(&gc);
     return false;
Feature-ACK, but code-NAK.
In key_method_2_read() there are a few places where goto error; can be used before up is allocated. This will make secure_memzero() segfault, as it does not handle NULL pointers. Or even worse (if the compiler let this pointer point at a random address), which leads to an unpredictable behaviour.

Quoted 4 lines

That said, I think we should fix secure_memzero() to just return if the input pointer is NULL. And even though most compilers do initialize variables, I think it's good to be defensive here and initialize up too.

Oof, good catch. I'll send a v2 in a minute.

-Steffan

Steffan Karger

author

2017-05-09 18:32 UTC #5 lore raw

PATCH v2 1/1 Always clear username/password from memory on error

This issue was found by Quarkslab during the OSTIF-founded security audit (issue 5.4), we are with their analysis:

Signed-off-by Steffan Karger <[email protected]...>

---
v2: only clear up if it is non-null

 src/openvpn/ssl.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

src/openvpn/ssl.c +5 -1

index 4fca283..21c9d6f 100644
@@ -2492,7 +2492,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio
 
     struct gc_arena gc = gc_new();
     char *options;
-    struct user_pass *up;
+    struct user_pass *up = NULL;
 
     /* allocate temporary objects */
     ALLOC_ARRAY_CLEAR_GC(options, char, TLS_OPTIONS_LEN, &gc);

@@ -2654,6 +2654,10 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio
 
 error:
     secure_memzero(ks->key_src, sizeof(*ks->key_src));
+    if (up)
+    {
+        secure_memzero(up, sizeof(*up));
+    }
     buf_clear(buf);
     gc_free(&gc);
     return false;

Signature · 1 line

2.7.4

David Sommerseth

2017-05-09 19:41 UTC #6 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK. This covers the concerns raised and avoids unexpected behaviours.

Your patch has been applied to the following branches

commit 2b60198e08a9d7e8de9beeb65a587ee34107efe8 (master) commit 47d80b95646a3cf62c9cd46ab589c442124c0474 (release/2.4)

Author: Steffan Karger
Date:   Tue May 9 20:32:44 2017 +0200

     Always clear username/password from memory on error

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: David Sommerseth <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://www.mail-archive.com/search?l=mid&[email protected]...
     Signed-off-by: David Sommerseth <[email protected]...>

- --
kind regards,

David Sommerseth

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBCAAGBQJZEhtsAAoJEIbPlEyWcf3yV+oP/i+1q3oISx2W/O6cBiQBRvJ8 oCzOY3P/VeFNJK2Z4KSG+PDDw5qt0/3AsrI9BHGhYAE98LeR7lcRkRCN/F8ta5KP /OYRHX1mh9X3tretR9GhnH6Zeab0K33Kqd+v8saxujlPz2uqnXnlAkpecgHZcdGe PmldC/CSJbcrGGIBgy7rWdhOytpWlQmb+DA/+aIOvfEGFT0aGwWtKSokZG+JNdPa OJBrj0zPBSDly/z1qirnFgmL2AYQtn1/1BO9ntUtUl8TrfT4cTaMnxMR0doGA/6o NbPGJuTOa6Vto2HNwyl2AC2MerJ5kvqXgRnifqFRGdB63/fh1HMuuQOf0ch+cOAz sZ6UboFLZjINicfO8nkap4ouDL6eynho2LWV5Qhfqq19qezif8KVMv+TtPepZMzK Emtva0fpOAltsVg/p/QzIp6XnmWcyYo0qmYN1YEFmMfCVB1kfUoglO3tghpUTVMv ZINB0AnNIpemvJ9sX8thqCyJcdlR69Sx1JcJz1RuAdzzrNtz7Spwm43PRWEsBSHO D0i5jb1KxGcRxLHwbKrfwVuPhP1D1elWYFphyKmY4qIB0BwLcysEh/QUZypKbrOw 6FGSLAJaMI6TL+ezB9YBrmKvLoQMx4TtqQDryXiR/zL5mGzjrmMzHpORBMXe4VlZ wxD9ALn6SSTN3Zydipjb
=AID1
-----END PGP SIGNATURE-----