From 0959d33ba0006e06d2dc5abfc45f4b91258d87c8 Mon Sep 17 00:00:00 2001 From: Adam Ierymenko Date: Tue, 21 Jun 2016 07:59:42 -0700 Subject: [PATCH] Add a few technically unnecessary but feel-good paranoia bounds checks in Dictionary.get(). --- node/Dictionary.hpp | 35 ++++++++++++++++++++++++++++------- 1 file changed, 28 insertions(+), 7 deletions(-) diff --git a/node/Dictionary.hpp b/node/Dictionary.hpp index 5c3c4f995..8a22b8aed 100644 --- a/node/Dictionary.hpp +++ b/node/Dictionary.hpp @@ -123,6 +123,12 @@ public: * C string in that case. The dest[] array will *never* be unterminated * after this call. * + * Security note: if 'key' is ever directly based on anything that is not + * a hard-code or internally-generated name, it must be checked to ensure + * that the buffer is NULL-terminated since key[] does not take a secondary + * size parameter. In NetworkConfig all keys are hard-coded strings so this + * isn't a problem in the core. + * * @param key Key to look up * @param dest Destination buffer * @param destlen Size of destination buffer @@ -131,6 +137,7 @@ public: inline int get(const char *key,char *dest,unsigned int destlen) const { const char *p = _d; + const char *const eof = p + C; const char *k; bool esc; int j; @@ -140,11 +147,14 @@ public: while (*p) { k = key; - while (*k) { + while ((*k)&&(*p)) { if (*p != *k) break; ++k; - ++p; + if (++p == eof) { + dest[0] = (char)0; + return -1; + } } if ((!*k)&&(*p == '=')) { @@ -174,15 +184,26 @@ public: return j-1; } } - ++p; + if (++p == eof) { + dest[0] = (char)0; + return -1; + } } dest[j] = (char)0; return j; } else { - while ((*p)&&(*p != '\r')&&(*p != '\n')) - ++p; - if (*p) - ++p; + while ((*p)&&(*p != '\r')&&(*p != '\n')) { + if (++p == eof) { + dest[0] = (char)0; + return -1; + } + } + if (*p) { + if (++p == eof) { + dest[0] = (char)0; + return -1; + } + } else break; } }