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.

1 files changed, 7 insertions, 0 deletions

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()