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

diff --git a/src/Notes.cc b/src/Notes.cc
index 393960077c6cc9621481f232ab819cbc1242f449..78b3b03f154afec11e8b6c46cd5ebf1a133d77c6 100644 (file)
--- 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 &noteKey)
 {
@@ -142,18 +173,19 @@ Notes::find(const SBuf &noteKey)
     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 31f377b77e01fb7d67b54e4058395394403fa1ee..1739128bdde2f3e6b4e6023357f8833ee1a4626f 100644 (file)
--- a/src/Notes.h
+++ b/src/Notes.h
@@ -109,12 +109,13 @@ class Notes : public RefCountable
 {
 public:
     typedef RefCount<Notes> Pointer;
+    typedef std::vector<SBuf> Keys; ///< unordered annotation names
     typedef std::vector<Note::Pointer> 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 &noteKey);
 
     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 a9e3947c0322164b5610c088235097b1eb83e854..1212588683ad4134b1dc9fd5311b3ce9a0f66d7b 100644 (file)
--- 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 5480cb05810f387dc02e95556f49ce1709e413a8..ab586760a10307e6f6ad00ffc237ba80090138dc 100644 (file)
--- 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*