From 4668df3dc310cd1bbb9da9b7498d918c8c2bc9c0 Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Sat, 29 Feb 2020 11:37:02 +0000 Subject: [PATCH] Ban reserved annotations in "note", "adaptation_meta" directives (#561) Squid defines a list of names used internally for exchanging name=value pairs with various helpers. When Squid receives a name=value pair with a reserved name, Squid stores it as if it was any other annotation; the information can be checked with a "note" ACL and logged with %note. An admin who configures a custom annotation with the same reserved name may see strange/unexpected results such as seemingly corrupted access.log record fields and mismatching ACLs. Squid already prohibits reserved annotation names in annotate_transaction and annotate_client ACLs. This change adds the missing protection to the "note" and adaptation_meta directives. This is a Measurement Factory project --- src/Notes.cc | 50 ++++++++++++++++++++++++++++++++------- src/Notes.h | 17 ++++++++----- src/acl/AnnotationData.cc | 17 +------------ src/adaptation/Config.cc | 24 +++++++++---------- 4 files changed, 65 insertions(+), 43 deletions(-) diff --git a/src/Notes.cc b/src/Notes.cc index 393960077c..78b3b03f15 100644 --- a/src/Notes.cc +++ b/src/Notes.cc @@ -17,6 +17,7 @@ #include "HttpReply.h" #include "HttpRequest.h" #include "parser/Tokenizer.h" +#include "sbuf/Stream.h" #include "sbuf/StringConvert.h" #include "SquidConfig.h" #include "Store.h" @@ -124,6 +125,36 @@ Note::toString(const char *sep) const return result; } +const Notes::Keys & +Notes::BlackList() +{ + // these keys are used for internal Squid-helper communication + static const char *names[] = { + "group", + "ha1", + "log", + "message", + "password", + "rewrite-url", + "status", + "tag", + "ttl", + "url", + "user" + }; + + static Keys keys(std::begin(names), std::end(names)); + return keys; +} + +Notes::Notes(const char *aDescr, const Keys *extraBlacklist, bool allowFormatted): + descr(aDescr), + formattedValues(allowFormatted) +{ + if (extraBlacklist) + blacklist = *extraBlacklist; +} + Note::Pointer Notes::add(const SBuf ¬eKey) { @@ -142,18 +173,19 @@ Notes::find(const SBuf ¬eKey) return nullptr; } +void +Notes::banReservedKey(const SBuf &key, const Keys &banned) const +{ + if (std::find(banned.begin(), banned.end(), key) != banned.end()) + throw TextException(ToSBuf("cannot use a reserved ", descr, " name: ", key), Here()); +} + void Notes::validateKey(const SBuf &key) const { - if (blacklisted) { - for (int i = 0; blacklisted[i] != nullptr; ++i) { - if (!key.cmp(blacklisted[i])) { - fatalf("%s:%d: meta key \"%.*s\" is a reserved %s name", - cfg_filename, config_lineno, key.length(), key.rawContent(), - descr ? descr : ""); - } - } - } + banReservedKey(key, BlackList()); + banReservedKey(key, blacklist); + // TODO: fix code duplication: the same set of specials is produced // by isKeyNameChar(). static const CharacterSet allowedSpecials = CharacterSet::ALPHA + diff --git a/src/Notes.h b/src/Notes.h index 31f377b77e..1739128bdd 100644 --- a/src/Notes.h +++ b/src/Notes.h @@ -109,12 +109,13 @@ class Notes : public RefCountable { public: typedef RefCount Pointer; + typedef std::vector Keys; ///< unordered annotation names typedef std::vector NotesList; typedef NotesList::iterator iterator; ///< iterates over the notes list typedef NotesList::const_iterator const_iterator; ///< iterates over the notes list - Notes(const char *aDescr, const char **metasBlacklist, bool allowFormatted = true): descr(aDescr), blacklisted(metasBlacklist), formattedValues(allowFormatted) {} - Notes(): descr(nullptr), blacklisted(nullptr), formattedValues(false) {} + explicit Notes(const char *aDescr, const Keys *extraBlacklist = nullptr, bool allowFormatted = true); + Notes() = default; ~Notes() { notes.clear(); } Notes(const Notes&) = delete; Notes &operator=(const Notes&) = delete; @@ -142,10 +143,11 @@ public: void updateNotePairs(NotePairsPointer pairs, const CharacterSet *delimiters, const AccessLogEntryPointer &al); private: + /// Makes sure the given key is not on the given list of banned names. + void banReservedKey(const SBuf &key, const Keys &banned) const; /// Verifies that the key is not blacklisted (fatal error) and /// does not contain special characters (non-fatal error). - /// If keyLen is not provided, the key is assumed null-terminated. void validateKey(const SBuf &key) const; /// Adds a note to the notes list and returns a pointer to the @@ -156,9 +158,12 @@ private: Note::Pointer find(const SBuf ¬eKey); NotesList notes; ///< The Note::Pointer objects array list - const char *descr; ///< A short description for notes list - const char **blacklisted; ///< Null terminated list of blacklisted note keys - bool formattedValues; ///< Whether the formatted values are supported + const char *descr = nullptr; ///< identifies note source in error messages + + Keys blacklist; ///< a list of additional prohibited key names + bool formattedValues = false; ///< whether to expand quoted logformat %codes + + static const Notes::Keys &BlackList(); ///< always prohibited key names }; /** diff --git a/src/acl/AnnotationData.cc b/src/acl/AnnotationData.cc index a9e3947c03..1212588683 100644 --- a/src/acl/AnnotationData.cc +++ b/src/acl/AnnotationData.cc @@ -16,23 +16,8 @@ #include "format/Format.h" #include "sbuf/Algorithms.h" -const char *AnnotationBlackList[] = { - "user", - "group", - "password", - "status", - "message", - "log", - "tag", - "ttl", - "ha1", - "rewrite-url", - "url", - nullptr -}; - ACLAnnotationData::ACLAnnotationData() - : notes(new Notes("annotation_data", AnnotationBlackList)) {} + : notes(new Notes("annotation_data")) {} SBufList ACLAnnotationData::dump() const diff --git a/src/adaptation/Config.cc b/src/adaptation/Config.cc index 5480cb0581..ab586760a1 100644 --- a/src/adaptation/Config.cc +++ b/src/adaptation/Config.cc @@ -28,24 +28,24 @@ int Adaptation::Config::service_iteration_limit = 16; int Adaptation::Config::send_client_ip = false; int Adaptation::Config::send_username = false; int Adaptation::Config::use_indirect_client = true; -const char *metasBlacklist[] = { - "Methods", - "Service", - "ISTag", +static const char *protectedFieldNamesRaw[] = { + "Allow", + "Date", "Encapsulated", - "Opt-body-type", + "ISTag", "Max-Connections", + "Methods", + "Opt-body-type", "Options-TTL", - "Date", - "Service-ID", - "Allow", "Preview", - "Transfer-Preview", - "Transfer-Ignore", + "Service", + "Service-ID", "Transfer-Complete", - NULL + "Transfer-Ignore", + "Transfer-Preview" }; -Notes Adaptation::Config::metaHeaders("ICAP header", metasBlacklist); +static const Notes::Keys protectedFieldNames(std::begin(protectedFieldNamesRaw), std::end(protectedFieldNamesRaw)); +Notes Adaptation::Config::metaHeaders("ICAP header", &protectedFieldNames); bool Adaptation::Config::needHistory = false; Adaptation::ServiceConfig* -- 2.47.2