From 204a091765c6717c3c7a01362afcf1de385b3d87 Mon Sep 17 00:00:00 2001 From: Timothy Flynn Date: Tue, 19 Oct 2021 09:14:05 -0400 Subject: [PATCH] LibCore: Avoid buffer overrun when invoking crypt() with a SecretString For example, consider the following SecretString construction: String foo = "foo"; auto ss = SecretString::take_ownership(foo.to_byte_buffer()); The ByteBuffer created by to_byte_buffer() will not contain the NUL terminator. Therefore, the value returned by SecretString::characters will not be NUL-terminated either. Currently, the only use of SecretString is to pass its character data to crypt(), which requires a NUL-terminated string. To ensure this cannot result in a buffer overrun, make SecretString append a NUL terminator to its buffer if there isn't one already. --- Userland/Libraries/LibCore/SecretString.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Userland/Libraries/LibCore/SecretString.cpp b/Userland/Libraries/LibCore/SecretString.cpp index f8cbb3392e..4763936a81 100644 --- a/Userland/Libraries/LibCore/SecretString.cpp +++ b/Userland/Libraries/LibCore/SecretString.cpp @@ -30,6 +30,13 @@ SecretString SecretString::take_ownership(ByteBuffer&& buffer) SecretString::SecretString(ByteBuffer&& buffer) : m_secure_buffer(move(buffer)) { + // SecretString is currently only used to provide the character data to invocations to crypt(), + // which requires a NUL-terminated string. To ensure this operation avoids a buffer overrun, + // append a NUL terminator here if there isn't already one. + if (m_secure_buffer.is_empty() || (m_secure_buffer[m_secure_buffer.size() - 1] != 0)) { + u8 nul = '\0'; + m_secure_buffer.append(&nul, 1); + } } SecretString::~SecretString()