diff --git a/CHANGES b/CHANGES index 4087615fe2..a50dc265cc 100644 --- a/CHANGES +++ b/CHANGES @@ -1,4 +1,15 @@ +2.3-396 | 2015-01-23 10:49:15 -0600 + + * DNP3: fix reachable assertion and buffer over-read/overflow. + CVE number pending. (Travis Emmert, Jon Siwek) + + * Update binpac: Fix potential out-of-bounds memory reads in generated + code. CVE-2014-9586. (John Villamil and Chris Rohlf - Yahoo + Paranoids, Jon Siwek) + + * Fixing (harmless) Coverity warning. (Robin Sommer) + 2.3-392 | 2015-01-15 09:44:15 -0800 * Small changes to EC curve names in a newer draft. (Johanna Amann) diff --git a/VERSION b/VERSION index 1cb805162d..081c98cc51 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -2.3-392 +2.3-396 diff --git a/src/analyzer/protocol/dnp3/DNP3.cc b/src/analyzer/protocol/dnp3/DNP3.cc index 135100eb6b..b04dbf64e0 100644 --- a/src/analyzer/protocol/dnp3/DNP3.cc +++ b/src/analyzer/protocol/dnp3/DNP3.cc @@ -138,9 +138,14 @@ bool DNP3_Base::ProcessData(int len, const u_char* data, bool orig) if ( endp->in_hdr ) { // We're parsing the DNP3 header and link layer, get that in full. - if ( ! AddToBuffer(endp, PSEUDO_APP_LAYER_INDEX, &data, &len) ) + int res = AddToBuffer(endp, PSEUDO_APP_LAYER_INDEX, &data, &len); + + if ( res == 0 ) return true; + if ( res < 0 ) + return false; + // The first two bytes must always be 0x0564. if( endp->buffer[0] != 0x05 || endp->buffer[1] != 0x64 ) { @@ -186,7 +191,11 @@ bool DNP3_Base::ProcessData(int len, const u_char* data, bool orig) if ( ! endp->in_hdr ) { - assert(endp->pkt_length); + if ( endp->pkt_length <= 0 ) + { + analyzer->Weird("dnp3_negative_or_zero_length_link_layer"); + return false; + } // We're parsing the DNP3 application layer, get that // in full now as well. We calculate the number of @@ -197,9 +206,14 @@ bool DNP3_Base::ProcessData(int len, const u_char* data, bool orig) int n = PSEUDO_APP_LAYER_INDEX + (endp->pkt_length - 5) + ((endp->pkt_length - 5) / 16) * 2 + 2 * ( ((endp->pkt_length - 5) % 16 == 0) ? 0 : 1) - 1 ; - if ( ! AddToBuffer(endp, n, &data, &len) ) + int res = AddToBuffer(endp, n, &data, &len); + + if ( res == 0 ) return true; + if ( res < 0 ) + return false; + // Parse the the application layer data. if ( ! ParseAppLayer(endp) ) return false; @@ -213,19 +227,42 @@ bool DNP3_Base::ProcessData(int len, const u_char* data, bool orig) return true; } -bool DNP3_Base::AddToBuffer(Endpoint* endp, int target_len, const u_char** data, int* len) +int DNP3_Base::AddToBuffer(Endpoint* endp, int target_len, const u_char** data, int* len) { if ( ! target_len ) - return true; + return 1; + + if ( *len < 0 ) + { + reporter->AnalyzerError(analyzer, "dnp3 negative input length: %d", *len); + return -1; + } + + if ( target_len < endp->buffer_len ) + { + reporter->AnalyzerError(analyzer, "dnp3 invalid target length: %d - %d", + target_len, endp->buffer_len); + return -1; + } int to_copy = min(*len, target_len - endp->buffer_len); + if ( endp->buffer_len + to_copy > MAX_BUFFER_SIZE ) + { + reporter->AnalyzerError(analyzer, "dnp3 buffer length exceeded: %d + %d", + endp->buffer_len, to_copy); + return -1; + } + memcpy(endp->buffer + endp->buffer_len, *data, to_copy); *data += to_copy; *len -= to_copy; endp->buffer_len += to_copy; - return endp->buffer_len == target_len; + if ( endp->buffer_len == target_len ) + return 1; + + return 0; } bool DNP3_Base::ParseAppLayer(Endpoint* endp) @@ -256,8 +293,15 @@ bool DNP3_Base::ParseAppLayer(Endpoint* endp) if ( ! CheckCRC(n, data, data + n, "app_chunk") ) return false; + if ( data + n >= endp->buffer + endp->buffer_len ) + { + reporter->AnalyzerError(analyzer, + "dnp3 app layer parsing overflow %d - %d", + endp->buffer_len, n); + return false; + } + // Pass on to BinPAC. - assert(data + n < endp->buffer + endp->buffer_len); flow->flow_buffer()->BufferData(data + transport, data + n); transport = 0; diff --git a/src/analyzer/protocol/dnp3/DNP3.h b/src/analyzer/protocol/dnp3/DNP3.h index 12c3624cd5..aa4ef78479 100644 --- a/src/analyzer/protocol/dnp3/DNP3.h +++ b/src/analyzer/protocol/dnp3/DNP3.h @@ -31,7 +31,21 @@ protected: bool ProcessData(int len, const u_char* data, bool orig); void ClearEndpointState(bool orig); - bool AddToBuffer(Endpoint* endp, int target_len, const u_char** data, int* len); + + /** + * Buffers packet data until it reaches a specified length. + * @param endp an endpoint speaking DNP3 to which data will be buffered. + * @param target_len the required length of the buffer + * @param data source buffer to copy bytes from. Will be incremented + * by the number of bytes copied by this function. + * @param len the number of bytes available in \a data. Will be decremented + * by the number of bytes copied by this function. + * @return -1 if invalid input parameters were supplied, 0 if the endpoint's + * buffer is not yet \a target_len bytes in size, or 1 the buffer is the + * required size. + */ + int AddToBuffer(Endpoint* endp, int target_len, const u_char** data, int* len); + bool ParseAppLayer(Endpoint* endp); bool CheckCRC(int len, const u_char* data, const u_char* crc16, const char* where); unsigned int CalcCRC(int len, const u_char* data);