From e08534c4b667a84fc7c83cea10af5f1908969470 Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Fri, 20 Oct 2023 11:03:43 +0200 Subject: [PATCH] QSysInfo: sanitize /etc file parsing [1/N]: properly check endsWith('"') in unquote() When 3535f46374ccf8ad966e4b266c0dbd919646fded dropped the Q_ASSERT(end[-1] == '"') by reviewer request from the original 767beb7fff6f08f5574a193967f8f814970ac289 change, it didn't replace it with some other check, leaving the possibility open that we'd be passing an invalid range (end < begin) to QString::fromUtf8(), with unknown consequences. While technically a security problem, this has very low impact, since the files being parsed should contain "trusted content", being supposed to be owned by root, and not writable by mere mortals. I hasten to add, though, that the code doesn't check permissions of the files it reads. The bug existed with the original Q_ASSERT, too. Namely in release mode. And in debug mode, it would be a DOS. Port to QByteArrayView to get the more expressive API and as a prerequestite for follow-up (non-security) patches. Pick-to: 6.5 Change-Id: Ib79cdad8680861d1f11b6be9234695273d0a97c2 Reviewed-by: Ivan Solovev Reviewed-by: Thiago Macieira Reviewed-by: Volker Hilsheimer (cherry picked from commit c39bd86766ae4e9faa3eda89a486bd5534816b0d) Reviewed-by: Qt Cherry-pick Bot --- src/corelib/global/qsysinfo.cpp | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/corelib/global/qsysinfo.cpp b/src/corelib/global/qsysinfo.cpp index 20919d0865d..40d75b11a14 100644 --- a/src/corelib/global/qsysinfo.cpp +++ b/src/corelib/global/qsysinfo.cpp @@ -224,7 +224,7 @@ struct QUnixOSVersion QString prettyName; // $PRETTY_NAME $DISTRIB_DESCRIPTION }; -static QString unquote(const char *begin, const char *end) +static QString unquote(QByteArrayView str) { // man os-release says: // Variable assignment values must be enclosed in double @@ -235,10 +235,11 @@ static QString unquote(const char *begin, const char *end) // All strings should be in UTF-8 format, and non-printable // characters should not be used. It is not supported to // concatenate multiple individually quoted strings. - if (*begin == '"') - return QString::fromUtf8(begin + 1, end - begin - 2); - return QString::fromUtf8(begin, end - begin); + if (str.size() >= 2 && str.front() == '"' && str.back() == '"') + str = str.sliced(1).chopped(1); + return QString::fromUtf8(str); } + static QByteArray getEtcFileContent(const char *filename) { // we're avoiding QFile here @@ -279,19 +280,19 @@ static bool readEtcFile(QUnixOSVersion &v, const char *filename, if (line.startsWith(idKey)) { ptr += idKey.size(); - v.productType = unquote(ptr, eol); + v.productType = unquote({ptr, eol}); continue; } if (line.startsWith(prettyNameKey)) { ptr += prettyNameKey.size(); - v.prettyName = unquote(ptr, eol); + v.prettyName = unquote({ptr, eol}); continue; } if (line.startsWith(versionKey)) { ptr += versionKey.size(); - v.productVersion = unquote(ptr, eol); + v.productVersion = unquote({ptr, eol}); continue; } }