Magic-Mirror/gecko-dev
1
0
Fork 0
mirror of https://github.com/mozilla/gecko-dev.git synced 2024-11-23 12:51:06 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Bug 1881526 - Add tainting support to ScopedLogExtraInfo; r=dom-storage-reviewers,asuth,jari

Browse Source 
Any value returned by ScopedLogExtraInfo::GetExtraInfoMap now requires explicit
validation before it can be used. This patch also adjusts LogError to use the
tainting for `extraInfosString` as well which should reduce even more the
chance of accidentally reporting sensitive information.

Differential Revision: https://phabricator.services.mozilla.com/D203355
This commit is contained in:
Jan Varga 2024-03-07 06:34:01 +00:00
parent 7d456a2c36
commit 4691dc6f32
10 changed files with 108 additions and 65 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
dom/indexedDB/ActorsParentCommon.cpp
View File

@ -728,7 +728,7 @@ nsresult ExecuteSimpleSQLSequence(mozIStorageConnection& aConnection,
Span<const nsLiteralCString> aSQLCommands) {
for (const auto& aSQLCommand : aSQLCommands) {
const auto extraInfo = quota::ScopedLogExtraInfo{
quota::ScopedLogExtraInfo::kTagQuery, aSQLCommand};
quota::ScopedLogExtraInfo::kTagQueryTainted, aSQLCommand};
QM_TRY(MOZ_TO_RESULT(aConnection.ExecuteSimpleSQL(aSQLCommand)));
}

2
dom/localstorage/LSSnapshot.cpp
View File

@ -378,7 +378,7 @@ nsresult LSSnapshot::SetItem(const nsAString& aKey, const nsAString& aValue,
{
quota::ScopedLogExtraInfo scope{
quota::ScopedLogExtraInfo::kTagContext,
quota::ScopedLogExtraInfo::kTagContextTainted,
"dom::localstorage::LSSnapshot::SetItem::UpdateUsage"_ns};
QM_TRY(MOZ_TO_RESULT(UpdateUsage(delta)), QM_PROPAGATE, QM_NO_CLEANUP,
([]() {

2
dom/quota/ActorsParent.cpp
View File

@ -2376,7 +2376,7 @@ void QuotaManager::Shutdown() {
// Body of the function
ScopedLogExtraInfo scope{ScopedLogExtraInfo::kTagContext,
ScopedLogExtraInfo scope{ScopedLogExtraInfo::kTagContextTainted,
"dom::quota::QuotaManager::Shutdown"_ns};
// This must be called before `flagShutdownStarted`, it would fail otherwise.

4
dom/quota/CachingDatabaseConnection.cpp
View File

@ -40,8 +40,8 @@ CachingDatabaseConnection::GetCachedStatement(const nsACString& aQuery) {
auto stmt,
mCachedStatements.TryLookupOrInsertWith(
aQuery, [&]() -> Result<nsCOMPtr<mozIStorageStatement>, nsresult> {
const auto extraInfo =
ScopedLogExtraInfo{ScopedLogExtraInfo::kTagQuery, aQuery};
const auto extraInfo = ScopedLogExtraInfo{
ScopedLogExtraInfo::kTagQueryTainted, aQuery};
QM_TRY_RETURN(
MOZ_TO_RESULT_INVOKE_MEMBER_TYPED(

2
dom/quota/CachingDatabaseConnection.h
View File

@ -55,7 +55,7 @@ class CachingDatabaseConnection {
BorrowedStatement(NotNull<mozIStorageStatement*> aStatement,
const nsACString& aQuery)
: mozStorageStatementScoper(aStatement),
mExtraInfo{ScopedLogExtraInfo::kTagQuery, aQuery} {}
mExtraInfo{ScopedLogExtraInfo::kTagQueryTainted, aQuery} {}
ScopedLogExtraInfo mExtraInfo;
#else

84
dom/quota/QuotaCommon.cpp
View File

@ -374,14 +374,15 @@ void LogError(const nsACString& aExpr, const Maybe<nsresult> aMaybeRv,
return;
}
nsAutoCString context;
const Tainted<nsCString>* contextTaintedPtr = nullptr;
# ifdef QM_SCOPED_LOG_EXTRA_INFO_ENABLED
const auto& extraInfoMap = ScopedLogExtraInfo::GetExtraInfoMap();
if (const auto contextIt = extraInfoMap.find(ScopedLogExtraInfo::kTagContext);
if (const auto contextIt =
extraInfoMap.find(ScopedLogExtraInfo::kTagContextTainted);
contextIt != extraInfoMap.cend()) {
context = *contextIt->second;
contextTaintedPtr = contextIt->second;
}
# endif
@ -444,37 +445,46 @@ void LogError(const nsACString& aExpr, const Maybe<nsresult> aMaybeRv,
}
# endif
nsAutoCString extraInfosString;
auto extraInfosStringTainted = Tainted<nsAutoCString>([&] {
nsAutoCString extraInfosString;
if (!rvCode.IsEmpty()) {
extraInfosString.Append(" failed with resultCode "_ns + rvCode);
}
if (!rvCode.IsEmpty()) {
extraInfosString.Append(" failed with resultCode "_ns + rvCode);
}
if (!rvName.IsEmpty()) {
extraInfosString.Append(", resultName "_ns + rvName);
}
if (!rvName.IsEmpty()) {
extraInfosString.Append(", resultName "_ns + rvName);
}
# ifdef QM_ERROR_STACKS_ENABLED
if (!frameIdString.IsEmpty()) {
extraInfosString.Append(", frameId "_ns + frameIdString);
}
if (!frameIdString.IsEmpty()) {
extraInfosString.Append(", frameId "_ns + frameIdString);
}
if (!stackIdString.IsEmpty()) {
extraInfosString.Append(", stackId "_ns + stackIdString);
}
if (!stackIdString.IsEmpty()) {
extraInfosString.Append(", stackId "_ns + stackIdString);
}
if (!processIdString.IsEmpty()) {
extraInfosString.Append(", processId "_ns + processIdString);
}
if (!processIdString.IsEmpty()) {
extraInfosString.Append(", processId "_ns + processIdString);
}
# endif
# ifdef QM_SCOPED_LOG_EXTRA_INFO_ENABLED
for (const auto& item : extraInfoMap) {
extraInfosString.Append(", "_ns + nsDependentCString(item.first) + " "_ns +
*item.second);
}
for (const auto& item : extraInfoMap) {
const auto& valueTainted = *item.second;
extraInfosString.Append(
", "_ns + nsDependentCString(item.first) + " "_ns +
MOZ_NO_VALIDATE(valueTainted,
"It's okay to append any `extraInfoMap` value to "
"`extraInfosString`."));
}
# endif
return extraInfosString;
}());
const auto sourceFileRelativePath =
detail::MakeSourceFileRelativePath(aSourceFilePath);
@ -482,9 +492,14 @@ void LogError(const nsACString& aExpr, const Maybe<nsresult> aMaybeRv,
NS_DebugBreak(
NS_DEBUG_WARNING,
nsAutoCString("QM_TRY failure ("_ns + severityString + ")"_ns).get(),
(extraInfosString.IsEmpty() ? nsPromiseFlatCString(aExpr)
: static_cast<const nsCString&>(nsAutoCString(
aExpr + extraInfosString)))
(MOZ_NO_VALIDATE(extraInfosStringTainted,
"It's okay to check if `extraInfosString` is empty.")
.IsEmpty()
? nsPromiseFlatCString(aExpr)
: static_cast<const nsCString&>(nsAutoCString(
aExpr + MOZ_NO_VALIDATE(extraInfosStringTainted,
"It's okay to log `extraInfosString` "
"to stdout/console."))))
.get(),
nsPromiseFlatCString(sourceFileRelativePath).get(), aSourceFileLine);
# endif
@ -496,13 +511,16 @@ void LogError(const nsACString& aExpr, const Maybe<nsresult> aMaybeRv,
// reporting (instead of the browsing console).
// Another option is to keep the current check and rely on MOZ_LOG reporting
// in future once that's available.
if (!context.IsEmpty()) {
if (contextTaintedPtr) {
nsCOMPtr<nsIConsoleService> console =
do_GetService(NS_CONSOLESERVICE_CONTRACTID);
if (console) {
NS_ConvertUTF8toUTF16 message(
"QM_TRY failure ("_ns + severityString + ")"_ns + ": '"_ns + aExpr +
extraInfosString + "', file "_ns + sourceFileRelativePath + ":"_ns +
MOZ_NO_VALIDATE(
extraInfosStringTainted,
"It's okay to log `extraInfosString` to the browser console.") +
"', file "_ns + sourceFileRelativePath + ":"_ns +
IntToCString(aSourceFileLine));
// The concatenation above results in a message like:
@ -521,7 +539,9 @@ void LogError(const nsACString& aExpr, const Maybe<nsresult> aMaybeRv,
// telemetry (besides carrying information). Other tags (like query) don't
// enable logging to telemetry.
if (!context.IsEmpty()) {
if (contextTaintedPtr) {
const auto& contextTainted = *contextTaintedPtr;
// Do NOT CHANGE this if you don't know what you're doing.
// `extraInfoString` is not included in the telemetry event on purpose
@ -538,7 +558,11 @@ void LogError(const nsACString& aExpr, const Maybe<nsresult> aMaybeRv,
auto res = CopyableTArray<EventExtraEntry>{};
res.SetCapacity(9);
res.AppendElement(EventExtraEntry{"context"_ns, nsCString{context}});
res.AppendElement(EventExtraEntry{
"context"_ns,
MOZ_NO_VALIDATE(
contextTainted,
"Context has been data-reviewed for telemetry transmission.")});
# ifdef QM_ERROR_STACKS_ENABLED
if (!frameIdString.IsEmpty()) {

4
dom/quota/QuotaCommon.h
View File

@ -1712,8 +1712,8 @@ auto ExecuteInitialization(
const auto maybeScopedLogExtraInfo =
firstInitializationAttempt.Recorded()
? Nothing{}
: Some(ScopedLogExtraInfo{ScopedLogExtraInfo::kTagContext,
aContext});
: Some(ScopedLogExtraInfo{
ScopedLogExtraInfo::kTagContextTainted, aContext});
#endif
return std::forward<Func>(aFunc)(firstInitializationAttempt);

26
dom/quota/ScopedLogExtraInfo.cpp
View File

@ -9,19 +9,21 @@
namespace mozilla::dom::quota {
#ifdef QM_SCOPED_LOG_EXTRA_INFO_ENABLED
MOZ_THREAD_LOCAL(const nsACString*) ScopedLogExtraInfo::sQueryValue;
MOZ_THREAD_LOCAL(const nsACString*) ScopedLogExtraInfo::sContextValue;
MOZ_THREAD_LOCAL(const Tainted<nsCString>*)
ScopedLogExtraInfo::sQueryValueTainted;
MOZ_THREAD_LOCAL(const Tainted<nsCString>*)
ScopedLogExtraInfo::sContextValueTainted;
/* static */
auto ScopedLogExtraInfo::FindSlot(const char* aTag) {
// XXX For now, don't use a real map but just allow the known tag values.
if (aTag == kTagQuery) {
return &sQueryValue;
if (aTag == kTagQueryTainted) {
return &sQueryValueTainted;
}
if (aTag == kTagContext) {
return &sContextValue;
if (aTag == kTagContextTainted) {
return &sContextValueTainted;
}
MOZ_CRASH("Unknown tag!");
@ -51,20 +53,20 @@ ScopedLogExtraInfo::GetExtraInfoMap() {
// the caller(s).
ScopedLogExtraInfoMap map;
if (sQueryValue.get()) {
map.emplace(kTagQuery, sQueryValue.get());
if (sQueryValueTainted.get()) {
map.emplace(kTagQueryTainted, sQueryValueTainted.get());
}
if (sContextValue.get()) {
map.emplace(kTagContext, sContextValue.get());
if (sContextValueTainted.get()) {
map.emplace(kTagContextTainted, sContextValueTainted.get());
}
return map;
}
/* static */ void ScopedLogExtraInfo::Initialize() {
MOZ_ALWAYS_TRUE(sQueryValue.init());
MOZ_ALWAYS_TRUE(sContextValue.init());
MOZ_ALWAYS_TRUE(sQueryValueTainted.init());
MOZ_ALWAYS_TRUE(sContextValueTainted.init());
}
void ScopedLogExtraInfo::AddInfo() {

16
dom/quota/ScopedLogExtraInfo.h
View File

@ -12,6 +12,7 @@
#include <map>
#include "mozilla/Assertions.h"
#include "mozilla/Attributes.h"
#include "mozilla/Tainting.h"
#include "mozilla/ThreadLocal.h"
#include "nsString.h"
#include "nsXULAppAPI.h"
@ -19,8 +20,8 @@
namespace mozilla::dom::quota {
struct MOZ_STACK_CLASS ScopedLogExtraInfo {
static constexpr const char kTagQuery[] = "query";
static constexpr const char kTagContext[] = "context";
static constexpr const char kTagQueryTainted[] = "query";
static constexpr const char kTagContextTainted[] = "context";
#ifdef QM_SCOPED_LOG_EXTRA_INFO_ENABLED
private:
@ -41,18 +42,19 @@ struct MOZ_STACK_CLASS ScopedLogExtraInfo {
ScopedLogExtraInfo(const ScopedLogExtraInfo&) = delete;
ScopedLogExtraInfo& operator=(const ScopedLogExtraInfo&) = delete;
using ScopedLogExtraInfoMap = std::map<const char*, const nsACString*>;
using ScopedLogExtraInfoMap =
std::map<const char*, const Tainted<nsCString>*>;
static ScopedLogExtraInfoMap GetExtraInfoMap();
static void Initialize();
private:
const char* mTag;
const nsACString* mPreviousValue;
nsCString mCurrentValue;
const Tainted<nsCString>* mPreviousValue;
Tainted<nsCString> mCurrentValue;
static MOZ_THREAD_LOCAL(const nsACString*) sQueryValue;
static MOZ_THREAD_LOCAL(const nsACString*) sContextValue;
static MOZ_THREAD_LOCAL(const Tainted<nsCString>*) sQueryValueTainted;
static MOZ_THREAD_LOCAL(const Tainted<nsCString>*) sContextValueTainted;
void AddInfo();
#else

31
dom/quota/test/gtest/TestScopedLogExtraInfo.cpp
View File

@ -16,19 +16,23 @@ TEST(DOM_Quota_ScopedLogExtraInfo, AddAndRemove)
{
const auto extraInfo =
ScopedLogExtraInfo{ScopedLogExtraInfo::kTagQuery, text};
ScopedLogExtraInfo{ScopedLogExtraInfo::kTagQueryTainted, text};
#ifdef QM_SCOPED_LOG_EXTRA_INFO_ENABLED
const auto& extraInfoMap = ScopedLogExtraInfo::GetExtraInfoMap();
EXPECT_EQ(text, *extraInfoMap.at(ScopedLogExtraInfo::kTagQuery));
const auto& queryValueTainted =
*extraInfoMap.at(ScopedLogExtraInfo::kTagQueryTainted);
EXPECT_EQ(text, MOZ_NO_VALIDATE(queryValueTainted,
"It's ok to use query value in tests."));
#endif
}
#ifdef QM_SCOPED_LOG_EXTRA_INFO_ENABLED
const auto& extraInfoMap = ScopedLogExtraInfo::GetExtraInfoMap();
EXPECT_EQ(0u, extraInfoMap.count(ScopedLogExtraInfo::kTagQuery));
EXPECT_EQ(0u, extraInfoMap.count(ScopedLogExtraInfo::kTagQueryTainted));
#endif
}
@ -39,27 +43,38 @@ TEST(DOM_Quota_ScopedLogExtraInfo, Nested)
{
const auto extraInfo =
ScopedLogExtraInfo{ScopedLogExtraInfo::kTagQuery, text};
ScopedLogExtraInfo{ScopedLogExtraInfo::kTagQueryTainted, text};
{
const auto extraInfo =
ScopedLogExtraInfo{ScopedLogExtraInfo::kTagQuery, nestedText};
ScopedLogExtraInfo{ScopedLogExtraInfo::kTagQueryTainted, nestedText};
#ifdef QM_SCOPED_LOG_EXTRA_INFO_ENABLED
const auto& extraInfoMap = ScopedLogExtraInfo::GetExtraInfoMap();
EXPECT_EQ(nestedText, *extraInfoMap.at(ScopedLogExtraInfo::kTagQuery));
const auto& queryValueTainted =
*extraInfoMap.at(ScopedLogExtraInfo::kTagQueryTainted);
EXPECT_EQ(nestedText,
MOZ_NO_VALIDATE(queryValueTainted,
"It's ok to use query value in tests."));
#endif
}
#ifdef QM_SCOPED_LOG_EXTRA_INFO_ENABLED
const auto& extraInfoMap = ScopedLogExtraInfo::GetExtraInfoMap();
EXPECT_EQ(text, *extraInfoMap.at(ScopedLogExtraInfo::kTagQuery));
const auto& queryValueTainted =
*extraInfoMap.at(ScopedLogExtraInfo::kTagQueryTainted);
EXPECT_EQ(text, MOZ_NO_VALIDATE(queryValueTainted,
"It's ok to use query value in tests."));
#endif
}
#ifdef QM_SCOPED_LOG_EXTRA_INFO_ENABLED
const auto& extraInfoMap = ScopedLogExtraInfo::GetExtraInfoMap();
EXPECT_EQ(0u, extraInfoMap.count(ScopedLogExtraInfo::kTagQuery));
EXPECT_EQ(0u, extraInfoMap.count(ScopedLogExtraInfo::kTagQueryTainted));
#endif
}