diff options
| author | Marc Mutz <marc.mutz@qt.io> | 2023-10-20 11:03:43 +0200 |
|---|---|---|
| committer | Marc Mutz <marc.mutz@qt.io> | 2023-12-07 05:16:23 +0000 |
| commit | c39bd86766ae4e9faa3eda89a486bd5534816b0d (patch) | |
| tree | e560029625dbc1a67e6d8313354f8b895acfc96a | |
| parent | 3c0fdd7341ed4bff9b5f041e9f4646265d142303 (diff) | |
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.6 6.5
Change-Id: Ib79cdad8680861d1f11b6be9234695273d0a97c2
Reviewed-by: Ivan Solovev <ivan.solovev@qt.io>
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
Reviewed-by: Volker Hilsheimer <volker.hilsheimer@qt.io>
| -rw-r--r-- | src/corelib/global/qsysinfo.cpp | 15 |
1 files 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; } } |