From fd7f26eaf8b41da6b4131ee33d2c898a888b2044 Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Wed, 28 Nov 2012 10:19:46 +1300 Subject: [PATCH] Audit Review updates * guarantee that note values output by the HelperReply parser are "" nil-terminated string and not undefined String. * rename HelperReply::responseKeys to 'notes' * rename Notes::findByName() to find() * add Note::firstValue() to locate first value provided for a given key and present it as a char* terminated string * various documentation updates --- src/HelperReply.cc | 20 ++++++++++---------- src/HelperReply.h | 10 +++++----- src/Notes.cc | 4 ++-- src/Notes.h | 29 ++++++++++++++++++----------- src/auth/digest/UserRequest.cc | 8 ++++---- src/auth/negotiate/UserRequest.cc | 26 +++++++++++++------------- src/auth/ntlm/UserRequest.cc | 22 +++++++++++----------- src/dns.cc | 4 ++-- src/external_acl.cc | 12 ++++++------ src/redirect.cc | 4 ++-- src/ssl/helper.cc | 2 +- 11 files changed, 74 insertions(+), 67 deletions(-) diff --git a/src/HelperReply.cc b/src/HelperReply.cc index d12499a2e0..972d8c8456 100644 --- a/src/HelperReply.cc +++ b/src/HelperReply.cc @@ -54,11 +54,11 @@ HelperReply::parse(char *buf, size_t len) MemBuf authToken; authToken.init(); authToken.append(w1, strlen(w1)); - responseKeys.add("token",authToken.content()); + notes.add("token",authToken.content()); } else { // token field is mandatory on this response code result = HelperReply::BrokenHelper; - responseKeys.add("message","Missing 'token' data"); + notes.add("message","Missing 'token' data"); } } else if (!strncmp(p,"AF ",3)) { @@ -75,19 +75,19 @@ HelperReply::parse(char *buf, size_t len) MemBuf authToken; authToken.init(); authToken.append(w1, strlen(w1)); - responseKeys.add("token",authToken.content()); + notes.add("token",authToken.content()); MemBuf user; user.init(); user.append(w2,strlen(w2)); - responseKeys.add("user",user.content()); + notes.add("user",user.content()); } else if (w1 != NULL) { // NTLM "user" MemBuf user; user.init(); user.append(w1,strlen(w1)); - responseKeys.add("user",user.content()); + notes.add("user",user.content()); } } else if (!strncmp(p,"NA ",3)) { // NTLM fail-closed ERR response @@ -109,7 +109,7 @@ HelperReply::parse(char *buf, size_t len) // Hack for backward-compatibility: BH used to be a text message... if (other().hasContent() && result == HelperReply::BrokenHelper) { - responseKeys.add("message",other().content()); + notes.add("message",other().content()); modifiableOther().clean(); } } @@ -139,9 +139,9 @@ HelperReply::parseResponseKeys() char *v = strwordtok(NULL, &p); if (v != NULL && urlDecode && (p-v) > 2) // 1-octet %-escaped requires 3 bytes rfc1738_unescape(v); - String value = v; + const String value(v?v:""); // value can be empty, but must not be NULL - responseKeys.add(key, value); + notes.add(key, value); modifiableOther().consume(p - other().content()); modifiableOther().consumeWhitespacePrefix(); @@ -171,9 +171,9 @@ operator <<(std::ostream &os, const HelperReply &r) } // dump the helper key=pair "notes" list - if (r.responseKeys.notes.size() > 0) { + if (r.notes.notes.size() > 0) { os << ", notes={"; - for (Notes::NotesList::const_iterator m = r.responseKeys.notes.begin(); m != r.responseKeys.notes.end(); ++m) { + for (Notes::NotesList::const_iterator m = r.notes.notes.begin(); m != r.notes.notes.end(); ++m) { for (Note::Values::iterator v = (*m)->values.begin(); v != (*m)->values.end(); ++v) { os << ',' << (*m)->key << '=' << ConfigParser::QuoteString((*v)->value); } diff --git a/src/HelperReply.h b/src/HelperReply.h index 867cff833c..c67c080e2a 100644 --- a/src/HelperReply.h +++ b/src/HelperReply.h @@ -24,7 +24,7 @@ private: HelperReply &operator =(const HelperReply &r); public: - HelperReply() : result(HelperReply::Unknown), responseKeys(), whichServer(NULL) { + HelperReply() : result(HelperReply::Unknown), notes(), whichServer(NULL) { other_.init(1,1); other_.terminate(); } @@ -42,8 +42,8 @@ public: MemBuf &modifiableOther() const { return *const_cast(&other_); } /** parse a helper response line format: - * line := [ result ] *#( key-pair ) - * key-pair := OWS token '=' ( quoted-string | token ) + * line := [ result ] *#( kv-pair ) + * kv-pair := OWS token '=' ( quoted-string | token ) * * token are URL-decoded. * quoted-string are \-escape decoded and the quotes are stripped. @@ -60,12 +60,12 @@ public: BrokenHelper, // "BH" indicating failure due to helper internal problems. // result codes for backward compatibility with NTLM/Negotiate - // TODO: migrate to a variant of the above results with key-pair parameters + // TODO: migrate to a variant of the above results with kv-pair parameters TT } result; // list of key=value pairs the helper produced - Notes responseKeys; + Notes notes; /// for stateful replies the responding helper 'server' needs to be preserved across callbacks CbcPointer whichServer; diff --git a/src/Notes.cc b/src/Notes.cc index 783f42fbf1..2060528c48 100644 --- a/src/Notes.cc +++ b/src/Notes.cc @@ -73,7 +73,7 @@ Note::match(HttpRequest *request, HttpReply *reply) } Note::Pointer -Notes::findByName(const String ¬eKey) const +Notes::find(const String ¬eKey) const { typedef Notes::NotesList::const_iterator AMLI; for (AMLI i = notes.begin(); i != notes.end(); ++i) { @@ -94,7 +94,7 @@ Notes::add(const String ¬eKey, const String ¬eValue) Note::Pointer Notes::add(const String ¬eKey) { - Note::Pointer note = findByName(noteKey); + Note::Pointer note = find(noteKey); if (note == NULL) { note = new Note(noteKey); notes.push_back(note); diff --git a/src/Notes.h b/src/Notes.h index c93ce4ac94..abf105532c 100644 --- a/src/Notes.h +++ b/src/Notes.h @@ -48,6 +48,12 @@ public: * or NULL if none matches. */ const char *match(HttpRequest *request, HttpReply *reply); + + /** + * Returns the first value for this key or an empty string. + */ + const char *firstValue() const { return (values.size()>0&&values[0]->value.defined()?values[0]->value.termedBuf():""); } + String key; ///< The note key Values values; ///< The possible values list for the note }; @@ -84,9 +90,21 @@ public: /// return true if the notes list is empty bool empty() { return notes.empty(); } + /** + * Adds a note key and value to the notes list. + * If the key name already exists in list, add the given value to its set of values. + */ + void add(const String ¬eKey, const String ¬eValue); + + /** + * Returns a pointer to an existing Note with given key name or nil. + */ + Note::Pointer find(const String ¬eKey) const; + 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 + private: /** * Adds a note to the notes list and returns a pointer to the @@ -95,17 +113,6 @@ private: */ Note::Pointer add(const String ¬eKey); -public: - /** - * Adds a note key and value to the notes list. - * If the key name already exists in list, add the given value to its set of values. - */ - void add(const String ¬eKey, const String ¬eValue); - - /** - * Looks up a note by key name and returns a Note::Pointer to it - */ - Note::Pointer findByName(const String ¬eKey) const; }; class NotePairs : public HttpHeader diff --git a/src/auth/digest/UserRequest.cc b/src/auth/digest/UserRequest.cc index ea4c3e2df3..355cc47f9c 100644 --- a/src/auth/digest/UserRequest.cc +++ b/src/auth/digest/UserRequest.cc @@ -304,9 +304,9 @@ Auth::Digest::UserRequest::HandleReply(void *data, const HelperReply &reply) Auth::Digest::User *digest_user = dynamic_cast(auth_user_request->user().getRaw()); assert(digest_user != NULL); - Note::Pointer ha1Note = reply.responseKeys.findByName("ha1"); + Note::Pointer ha1Note = reply.notes.find("ha1"); if (ha1Note != NULL) { - CvtBin(ha1Note->values[0]->value.termedBuf(), digest_user->HA1); + CvtBin(ha1Note->firstValue(), digest_user->HA1); digest_user->HA1created = 1; } else { debugs(29, DBG_IMPORTANT, "ERROR: Digest auth helper did not produce a HA1. Using the wrong helper program? received: " << reply); @@ -330,9 +330,9 @@ Auth::Digest::UserRequest::HandleReply(void *data, const HelperReply &reply) digest_request->user()->credentials(Auth::Failed); digest_request->flags.invalid_password = 1; - Note::Pointer msgNote = reply.responseKeys.findByName("message"); + Note::Pointer msgNote = reply.notes.find("message"); if (msgNote != NULL) { - digest_request->setDenyMessage(msgNote->values[0]->value.termedBuf()); + digest_request->setDenyMessage(msgNote->firstValue()); } else if (reply.other().hasContent()) { // old helpers did send ERR result but a bare message string instead of message= key name. digest_request->setDenyMessage(reply.other().content()); diff --git a/src/auth/negotiate/UserRequest.cc b/src/auth/negotiate/UserRequest.cc index bf03ce21c1..0f4a956638 100644 --- a/src/auth/negotiate/UserRequest.cc +++ b/src/auth/negotiate/UserRequest.cc @@ -268,11 +268,11 @@ Auth::Negotiate::UserRequest::HandleReply(void *data, const HelperReply &reply) safe_free(lm_request->server_blob); lm_request->request->flags.mustKeepalive = 1; if (lm_request->request->flags.proxyKeepalive) { - Note::Pointer tokenNote = reply.responseKeys.findByName("token"); - lm_request->server_blob = xstrdup(tokenNote->values[0]->value.termedBuf()); + Note::Pointer tokenNote = reply.notes.find("token"); + lm_request->server_blob = xstrdup(tokenNote->firstValue()); auth_user_request->user()->credentials(Auth::Handshake); auth_user_request->denyMessage("Authentication in progress"); - debugs(29, 4, HERE << "Need to challenge the client with a server token: '" << tokenNote->values[0]->value << "'"); + debugs(29, 4, HERE << "Need to challenge the client with a server token: '" << tokenNote->firstValue() << "'"); } else { auth_user_request->user()->credentials(Auth::Failed); auth_user_request->denyMessage("Negotiate authentication requires a persistent connection"); @@ -280,8 +280,8 @@ Auth::Negotiate::UserRequest::HandleReply(void *data, const HelperReply &reply) break; case HelperReply::Okay: { - Note::Pointer userNote = reply.responseKeys.findByName("user"); - Note::Pointer tokenNote = reply.responseKeys.findByName("token"); + Note::Pointer userNote = reply.notes.find("user"); + Note::Pointer tokenNote = reply.notes.find("token"); if (userNote == NULL || tokenNote == NULL) { // XXX: handle a success with no username better /* protocol error */ @@ -290,10 +290,10 @@ Auth::Negotiate::UserRequest::HandleReply(void *data, const HelperReply &reply) } /* we're finished, release the helper */ - auth_user_request->user()->username(userNote->values[0]->value.termedBuf()); + auth_user_request->user()->username(userNote->firstValue()); auth_user_request->denyMessage("Login successful"); safe_free(lm_request->server_blob); - lm_request->server_blob = xstrdup(tokenNote->values[0]->value.termedBuf()); + lm_request->server_blob = xstrdup(tokenNote->firstValue()); lm_request->releaseAuthServer(); /* connection is authenticated */ @@ -327,8 +327,8 @@ Auth::Negotiate::UserRequest::HandleReply(void *data, const HelperReply &reply) break; case HelperReply::Error: { - Note::Pointer messageNote = reply.responseKeys.findByName("message"); - Note::Pointer tokenNote = reply.responseKeys.findByName("token"); + Note::Pointer messageNote = reply.notes.find("message"); + Note::Pointer tokenNote = reply.notes.find("token"); if (tokenNote == NULL) { /* protocol error */ fatalf("authenticateNegotiateHandleReply: *** Unsupported helper response ***, '%s'\n", reply.other().content()); @@ -336,10 +336,10 @@ Auth::Negotiate::UserRequest::HandleReply(void *data, const HelperReply &reply) } /* authentication failure (wrong password, etc.) */ - auth_user_request->denyMessage(messageNote->values[0]->value.termedBuf()); + auth_user_request->denyMessage(messageNote->firstValue()); auth_user_request->user()->credentials(Auth::Failed); safe_free(lm_request->server_blob); - lm_request->server_blob = xstrdup(tokenNote->values[0]->value.termedBuf()); + lm_request->server_blob = xstrdup(tokenNote->firstValue()); lm_request->releaseAuthServer(); debugs(29, 4, HERE << "Failed validating user via Negotiate. Error returned '" << reply << "'"); } @@ -355,11 +355,11 @@ Auth::Negotiate::UserRequest::HandleReply(void *data, const HelperReply &reply) * Authenticate Negotiate start. * If after a KK deny the user's request w/ 407 and mark the helper as * Needing YR. */ - Note::Pointer errNote = reply.responseKeys.findByName("message"); + Note::Pointer errNote = reply.notes.find("message"); if (reply.result == HelperReply::Unknown) auth_user_request->denyMessage("Internal Error"); else if (errNote != NULL) - auth_user_request->denyMessage(errNote->values[0]->value.termedBuf()); + auth_user_request->denyMessage(errNote->firstValue()); else auth_user_request->denyMessage("Negotiate Authentication failed with no reason given"); auth_user_request->user()->credentials(Auth::Failed); diff --git a/src/auth/ntlm/UserRequest.cc b/src/auth/ntlm/UserRequest.cc index 0c9cd97695..a11b9abd8c 100644 --- a/src/auth/ntlm/UserRequest.cc +++ b/src/auth/ntlm/UserRequest.cc @@ -261,11 +261,11 @@ Auth::Ntlm::UserRequest::HandleReply(void *data, const HelperReply &reply) safe_free(lm_request->server_blob); lm_request->request->flags.mustKeepalive = 1; if (lm_request->request->flags.proxyKeepalive) { - Note::Pointer serverBlob = reply.responseKeys.findByName("token"); - lm_request->server_blob = xstrdup(serverBlob->values[0]->value.termedBuf()); + Note::Pointer serverBlob = reply.notes.find("token"); + lm_request->server_blob = xstrdup(serverBlob->firstValue()); auth_user_request->user()->credentials(Auth::Handshake); auth_user_request->denyMessage("Authentication in progress"); - debugs(29, 4, HERE << "Need to challenge the client with a server token: '" << serverBlob->values[0]->value << "'"); + debugs(29, 4, HERE << "Need to challenge the client with a server token: '" << serverBlob->firstValue() << "'"); } else { auth_user_request->user()->credentials(Auth::Failed); auth_user_request->denyMessage("NTLM authentication requires a persistent connection"); @@ -274,13 +274,13 @@ Auth::Ntlm::UserRequest::HandleReply(void *data, const HelperReply &reply) case HelperReply::Okay: { /* we're finished, release the helper */ - Note::Pointer userLabel = reply.responseKeys.findByName("user"); - auth_user_request->user()->username(userLabel->values[0]->value.termedBuf()); + Note::Pointer userLabel = reply.notes.find("user"); + auth_user_request->user()->username(userLabel->firstValue()); auth_user_request->denyMessage("Login successful"); safe_free(lm_request->server_blob); lm_request->releaseAuthServer(); - debugs(29, 4, HERE << "Successfully validated user via NTLM. Username '" << userLabel->values[0]->value << "'"); + debugs(29, 4, HERE << "Successfully validated user via NTLM. Username '" << userLabel->firstValue() << "'"); /* connection is authenticated */ debugs(29, 4, HERE << "authenticated user " << auth_user_request->user()->username()); /* see if this is an existing user with a different proxy_auth @@ -313,15 +313,15 @@ Auth::Ntlm::UserRequest::HandleReply(void *data, const HelperReply &reply) case HelperReply::Error: { /* authentication failure (wrong password, etc.) */ - Note::Pointer errNote = reply.responseKeys.findByName("message"); + Note::Pointer errNote = reply.notes.find("message"); if (errNote != NULL) - auth_user_request->denyMessage(errNote->values[0]->value.termedBuf()); + auth_user_request->denyMessage(errNote->firstValue()); else auth_user_request->denyMessage("NTLM Authentication denied with no reason given"); auth_user_request->user()->credentials(Auth::Failed); safe_free(lm_request->server_blob); lm_request->releaseAuthServer(); - debugs(29, 4, HERE << "Failed validating user via NTLM. Error returned '" << errNote->values[0]->value << "'"); + debugs(29, 4, HERE << "Failed validating user via NTLM. Error returned '" << errNote->firstValue() << "'"); } break; @@ -335,11 +335,11 @@ Auth::Ntlm::UserRequest::HandleReply(void *data, const HelperReply &reply) * Authenticate NTLM start. * If after a KK deny the user's request w/ 407 and mark the helper as * Needing YR. */ - Note::Pointer errNote = reply.responseKeys.findByName("message"); + Note::Pointer errNote = reply.notes.find("message"); if (reply.result == HelperReply::Unknown) auth_user_request->denyMessage("Internal Error"); else if (errNote != NULL) - auth_user_request->denyMessage(errNote->values[0]->value.termedBuf()); + auth_user_request->denyMessage(errNote->firstValue()); else auth_user_request->denyMessage("NTLM Authentication failed with no reason given"); auth_user_request->user()->credentials(Auth::Failed); diff --git a/src/dns.cc b/src/dns.cc index 5c51a5343f..c0f9f15927 100644 --- a/src/dns.cc +++ b/src/dns.cc @@ -134,8 +134,8 @@ dnsSubmit(const char *lookup, HLPCB * callback, void *data) HelperReply failReply; /* XXX: upgrade the ipcache and fqdn cache handlers to new syntax failReply.result= HelperReply::BrokenHelper; - failReply.responseKeys.add("message","Temporary network problem, please retry later"); - failReply.responseKeys.add("message","DNS lookup queue overloaded"); + failReply.notes.add("message","Temporary network problem, please retry later"); + failReply.notes.add("message","DNS lookup queue overloaded"); */ failReply.modifiableOther().append(t, strlen(t)); callback(data, failReply); diff --git a/src/external_acl.cc b/src/external_acl.cc index a7d5640c31..c0c46bb973 100644 --- a/src/external_acl.cc +++ b/src/external_acl.cc @@ -1326,26 +1326,26 @@ externalAclHandleReply(void *data, const HelperReply &reply) entryData.result = ACCESS_ALLOWED; // XXX: handle other non-DENIED results better - // XXX: make entryData store a proper helperReply object. + // XXX: make entryData store a proper HelperReply object instead of copying. - Note::Pointer label = reply.responseKeys.findByName("tag"); + Note::Pointer label = reply.notes.find("tag"); if (label != NULL && label->values[0]->value.size() > 0) entryData.tag = label->values[0]->value; - label = reply.responseKeys.findByName("message"); + label = reply.notes.find("message"); if (label != NULL && label->values[0]->value.size() > 0) entryData.message = label->values[0]->value; - label = reply.responseKeys.findByName("log"); + label = reply.notes.find("log"); if (label != NULL && label->values[0]->value.size() > 0) entryData.log = label->values[0]->value; #if USE_AUTH - label = reply.responseKeys.findByName("user"); + label = reply.notes.find("user"); if (label != NULL && label->values[0]->value.size() > 0) entryData.user = label->values[0]->value; - label = reply.responseKeys.findByName("password"); + label = reply.notes.find("password"); if (label != NULL && label->values[0]->value.size() > 0) entryData.password = label->values[0]->value; #endif diff --git a/src/redirect.cc b/src/redirect.cc index a0eab3a057..3d176852e2 100644 --- a/src/redirect.cc +++ b/src/redirect.cc @@ -79,7 +79,7 @@ redirectHandleReply(void *data, const HelperReply &reply) debugs(61, 5, HERE << "reply=" << reply); // XXX: This funtion is now kept only to check for and display this garbage use-case - // it can be removed when the helpers are all updated to the normalized "OK/ERR key-pairs" format + // it can be removed when the helpers are all updated to the normalized "OK/ERR kv-pairs" format if (reply.result == HelperReply::Unknown) { // BACKWARD COMPATIBILITY 2012-06-15: @@ -153,7 +153,7 @@ redirectStart(ClientHttpRequest * http, HLPCB * handler, void *data) ++n_bypassed; HelperReply bypassReply; bypassReply.result = HelperReply::Okay; - bypassReply.responseKeys.add("message","URL rewrite/redirect queue too long. Bypassed."); + bypassReply.notes.add("message","URL rewrite/redirect queue too long. Bypassed."); handler(data, bypassReply); return; } diff --git a/src/ssl/helper.cc b/src/ssl/helper.cc index bf1153e093..068ddae560 100644 --- a/src/ssl/helper.cc +++ b/src/ssl/helper.cc @@ -95,7 +95,7 @@ void Ssl::Helper::sslSubmit(CrtdMessage const & message, HLPCB * callback, void debugs(34, DBG_IMPORTANT, HERE << "Queue overload, rejecting"); HelperReply failReply; failReply.result = HelperReply::BrokenHelper; - failReply.responseKeys.add("message", "error 45 Temporary network problem, please retry later"); + failReply.notes.add("message", "error 45 Temporary network problem, please retry later"); callback(data, failReply); return; } -- 2.47.2