Mageia Bugzilla – Attachment 7494 Details for
Bug 17816
squid new security issue SQUID-2016_2
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
Log In
[x]
|
New Account
|
Forgot Password
[patch]
Patch squid-3.5-13991.patch backported to 3.4
squid-3.4-13991.patch (text/plain), 8.29 KB, created by
Nicolas Salguero
on 2016-02-26 14:03:47 CET
(
hide
)
Description:
Patch squid-3.5-13991.patch backported to 3.4
Filename:
MIME Type:
Creator:
Nicolas Salguero
Created:
2016-02-26 14:03:47 CET
Size:
8.29 KB
patch
obsolete
>------------------------------------------------------------ >revno: 13991 >revision-id: rousskov@measurement-factory.com-20160219231541-syrgnvl1av8bbn8d >parent: rousskov@measurement-factory.com-20160218041533-8tmtd45c3nky2gyy >committer: Alex Rousskov <rousskov@measurement-factory.com> >branch nick: 3.5 >timestamp: Fri 2016-02-19 16:15:41 -0700 >message: > Throw instead of asserting on some String overflows. > > Note that Client-caught exceptions result in HTTP 500 (Internal Server > Error) responses with X-Squid-Error set to "ERR_CANNOT_FORWARD 0". > > Also avoid stuck Client jobs on exceptions. > > Also unified String size limit checks. > > Essentially trunk r14552, which has a detailed commit message. >------------------------------------------------------------ ># Bazaar merge directive format 2 (Bazaar 0.90) ># revision_id: rousskov@measurement-factory.com-20160219231541-\ ># syrgnvl1av8bbn8d ># target_branch: http://bzr.squid-cache.org/bzr/squid3/3.5 ># testament_sha1: 3a9c41e0584065e737250cf9f8eb9eea7a85e9ba ># timestamp: 2016-02-19 23:50:57 +0000 ># source_branch: http://bzr.squid-cache.org/bzr/squid3/3.5 ># base_revision_id: rousskov@measurement-factory.com-20160218041533-\ ># 8tmtd45c3nky2gyy ># ># Begin patch >=== modified file 'src/SquidString.h' >--- src/SquidString.h 2016-01-01 00:14:27 +0000 >+++ src/SquidString.h 2016-02-19 23:15:41 +0000 >@@ -146,6 +146,13 @@ > _SQUID_INLINE_ int caseCmp(char const *, size_type count) const; > _SQUID_INLINE_ int caseCmp(String const &) const; > >+ /// Whether creating a totalLen-character string is safe (i.e., unlikely to assert). >+ /// Optional extras can be used for overflow-safe length addition. >+ /// Implementation has to add 1 because many String allocation methods do. >+ static bool CanGrowTo(size_type totalLen, const size_type extras = 0) { return SafeAdd(totalLen, extras) && SafeAdd(totalLen, 1); } >+ /// whether appending growthLen characters is safe (i.e., unlikely to assert) >+ bool canGrowBy(const size_type growthLen) const { return CanGrowTo(size(), growthLen); } >+ > String substr(size_type from, size_type to) const; > > _SQUID_INLINE_ void cut(size_type newLength); >@@ -162,10 +169,14 @@ > _SQUID_INLINE_ bool nilCmp(bool, bool, int &) const; > > /* never reference these directly! */ >- size_type size_; /* buffer size; 64K limit */ >+ size_type size_; /* buffer size; limited by SizeMax_ */ > > size_type len_; /* current length */ > >+ static const size_type SizeMax_ = 65535; ///< 64K limit protects some fixed-size buffers >+ /// returns true after increasing the first argument by extra if the sum does not exceed SizeMax_ >+ static bool SafeAdd(size_type &base, size_type extra) { if (extra <= SizeMax_ && base <= SizeMax_ - extra) { base += extra; return true; } return false; } >+ > char *buf_; > > _SQUID_INLINE_ void set(char const *loc, char const ch); > >=== modified file 'src/StrList.cc' >--- src/StrList.cc 2016-01-01 00:14:27 +0000 >+++ src/StrList.cc 2016-02-19 23:15:41 +0000 >@@ -33,20 +33,24 @@ > #include "squid.h" > #include "SquidString.h" > #include "StrList.h" >+#include "base/TextException.h" > > /** appends an item to the list */ > void > strListAdd(String * str, const char *item, char del) > { > assert(str && item); >+ const String::size_type itemSize = strlen(item); > if (str->size()) { > char buf[3]; > buf[0] = del; > buf[1] = ' '; > buf[2] = '\0'; >+ Must(str->canGrowBy(2)); > str->append(buf, 2); > } >- str->append(item, strlen(item)); >+ Must(str->canGrowBy(itemSize)); >+ str->append(item, itemSize); > } > > /** returns true iff "m" is a member of the list */ > >=== modified file 'src/String.cc' >--- src/String.cc 2016-01-01 00:14:27 +0000 >+++ src/String.cc 2016-02-19 23:15:41 +0000 >@@ -67,7 +67,7 @@ > String::setBuffer(char *aBuf, String::size_type aSize) > { > assert(undefined()); >- assert(aSize < 65536); >+ assert(aSize <= SizeMax_); > buf_ = aBuf; > size_ = aSize; > } >@@ -198,7 +198,7 @@ > } else { > // Create a temporary string and absorb it later. > String snew; >- assert(len_ + len < 65536); // otherwise snew.len_ overflows below >+ assert(canGrowBy(len)); // otherwise snew.len_ may overflow below > snew.len_ = len_ + len; > snew.allocBuffer(snew.len_ + 1); > > >=== modified file 'src/Server.cc' >--- src/Server.cc 2016-01-31 05:39:09 +0000 >+++ src/Server.cc 2016-02-19 23:15:41 +0000 >@@ -68,6 +68,7 @@ > startedAdaptation(false), > #endif > receivedWholeRequestBody(false), >+ doneWithFwd(NULL), > theVirginReply(NULL), > theFinalReply(NULL) > { >@@ -95,8 +96,6 @@ > HTTPMSGUNLOCK(theVirginReply); > HTTPMSGUNLOCK(theFinalReply); > >- fwd = NULL; // refcounted >- > if (responseBodyBuffer != NULL) { > delete responseBodyBuffer; > responseBodyBuffer = NULL; >@@ -114,6 +113,14 @@ > cleanAdaptation(); > #endif > >+ if (!doneWithServer()) >+ closeServer(); >+ >+ if (!doneWithFwd) { >+ doneWithFwd = "swanSong()"; >+ fwd->handleUnregisteredServerEnd(); >+ } >+ > BodyConsumer::swanSong(); > #if USE_ADAPTATION > Initiator::swanSong(); >@@ -238,6 +246,7 @@ > { > debugs(11,5, HERE << "completing forwarding for " << fwd); > assert(fwd != NULL); >+ doneWithFwd = "completeForwarding()"; > fwd->complete(); > } > > >=== modified file 'src/Server.h' >--- src/Server.h 2016-01-31 05:39:09 +0000 >+++ src/Server.h 2016-02-19 23:15:41 +0000 >@@ -192,6 +192,10 @@ > #endif > bool receivedWholeRequestBody; ///< handleRequestBodyProductionEnded called > >+ /// whether we should not be talking to FwdState; XXX: clear fwd instead >+ /// points to a string literal which is used only for debugging >+ const char *doneWithFwd; >+ > private: > void sendBodyIsTooLargeError(); > void maybePurgeOthers(); > >=== modified file 'src/ftp.cc' >--- src/ftp.cc 2016-01-31 05:39:09 +0000 >+++ src/ftp.cc 2016-02-19 23:15:41 +0000 >@@ -445,6 +445,7 @@ > { > debugs(9, 4, HERE); > ctrl.clear(); >+ doneWithFwd = "ctrlClosed()"; // assume FwdState is monitoring too > mustStop("FtpStateData::ctrlClosed"); > } > >@@ -3853,26 +3854,14 @@ > > return true; > } > >-/** >- * Quickly abort the transaction >- * >- \todo destruction should be sufficient as the destructor should cleanup, >- * including canceling close handlers >- */ > void > FtpStateData::abortTransaction(const char *reason) > { > debugs(9, 3, HERE << "aborting transaction for " << reason << > "; FD " << (ctrl.conn!=NULL?ctrl.conn->fd:-1) << ", Data FD " << (data.conn!=NULL?data.conn->fd:-1) << ", this " << this); >- if (Comm::IsConnOpen(ctrl.conn)) { >- ctrl.conn->close(); >- return; >- } >- >- fwd->handleUnregisteredServerEnd(); >- mustStop("FtpStateData::abortTransaction"); >+ mustStop(reason); > } > > /// creates a data channel Comm close callback > AsyncCall::Pointer >=== modified file 'src/http.cc' >--- src/http.cc 2016-02-18 04:15:33 +0000 >+++ src/http.cc 2016-02-19 23:15:41 +0000 >@@ -173,6 +173,7 @@ > HttpStateData::httpStateConnClosed(const CommCloseCbParams ¶ms) > { > debugs(11, 5, "httpStateFree: FD " << params.fd << ", httpState=" << params.data); >+ doneWithFwd = "httpStateConnClosed()"; // assume FwdState is monitoring too > mustStop("HttpStateData::httpStateConnClosed"); > } > >@@ -1757,7 +1758,8 @@ > > String strFwd = hdr_in->getList(HDR_X_FORWARDED_FOR); > >- if (strFwd.size() > 65536/2) { >+ // if we cannot double strFwd size, then it grew past 50% of the limit >+ if (!strFwd.canGrowBy(strFwd.size())) { > // There is probably a forwarding loop with Via detection disabled. > // If we do nothing, String will assert on overflow soon. > // TODO: Terminate all transactions with huge XFF? >@@ -2408,20 +2410,10 @@ > ServerStateData::sentRequestBody(io); > } > >-// Quickly abort the transaction >-// TODO: destruction should be sufficient as the destructor should cleanup, >-// including canceling close handlers > void > HttpStateData::abortTransaction(const char *reason) > { > debugs(11,5, HERE << "aborting transaction for " << reason << > "; " << serverConnection << ", this " << this); >- >- if (Comm::IsConnOpen(serverConnection)) { >- serverConnection->close(); >- return; >- } >- >- fwd->handleUnregisteredServerEnd(); >- mustStop("HttpStateData::abortTransaction"); >+ mustStop(reason); > }
------------------------------------------------------------ revno: 13991 revision-id: rousskov@measurement-factory.com-20160219231541-syrgnvl1av8bbn8d parent: rousskov@measurement-factory.com-20160218041533-8tmtd45c3nky2gyy committer: Alex Rousskov <rousskov@measurement-factory.com> branch nick: 3.5 timestamp: Fri 2016-02-19 16:15:41 -0700 message: Throw instead of asserting on some String overflows. Note that Client-caught exceptions result in HTTP 500 (Internal Server Error) responses with X-Squid-Error set to "ERR_CANNOT_FORWARD 0". Also avoid stuck Client jobs on exceptions. Also unified String size limit checks. Essentially trunk r14552, which has a detailed commit message. ------------------------------------------------------------ # Bazaar merge directive format 2 (Bazaar 0.90) # revision_id: rousskov@measurement-factory.com-20160219231541-\ # syrgnvl1av8bbn8d # target_branch: http://bzr.squid-cache.org/bzr/squid3/3.5 # testament_sha1: 3a9c41e0584065e737250cf9f8eb9eea7a85e9ba # timestamp: 2016-02-19 23:50:57 +0000 # source_branch: http://bzr.squid-cache.org/bzr/squid3/3.5 # base_revision_id: rousskov@measurement-factory.com-20160218041533-\ # 8tmtd45c3nky2gyy # # Begin patch === modified file 'src/SquidString.h' --- src/SquidString.h 2016-01-01 00:14:27 +0000 +++ src/SquidString.h 2016-02-19 23:15:41 +0000 @@ -146,6 +146,13 @@ _SQUID_INLINE_ int caseCmp(char const *, size_type count) const; _SQUID_INLINE_ int caseCmp(String const &) const; + /// Whether creating a totalLen-character string is safe (i.e., unlikely to assert). + /// Optional extras can be used for overflow-safe length addition. + /// Implementation has to add 1 because many String allocation methods do. + static bool CanGrowTo(size_type totalLen, const size_type extras = 0) { return SafeAdd(totalLen, extras) && SafeAdd(totalLen, 1); } + /// whether appending growthLen characters is safe (i.e., unlikely to assert) + bool canGrowBy(const size_type growthLen) const { return CanGrowTo(size(), growthLen); } + String substr(size_type from, size_type to) const; _SQUID_INLINE_ void cut(size_type newLength); @@ -162,10 +169,14 @@ _SQUID_INLINE_ bool nilCmp(bool, bool, int &) const; /* never reference these directly! */ - size_type size_; /* buffer size; 64K limit */ + size_type size_; /* buffer size; limited by SizeMax_ */ size_type len_; /* current length */ + static const size_type SizeMax_ = 65535; ///< 64K limit protects some fixed-size buffers + /// returns true after increasing the first argument by extra if the sum does not exceed SizeMax_ + static bool SafeAdd(size_type &base, size_type extra) { if (extra <= SizeMax_ && base <= SizeMax_ - extra) { base += extra; return true; } return false; } + char *buf_; _SQUID_INLINE_ void set(char const *loc, char const ch); === modified file 'src/StrList.cc' --- src/StrList.cc 2016-01-01 00:14:27 +0000 +++ src/StrList.cc 2016-02-19 23:15:41 +0000 @@ -33,20 +33,24 @@ #include "squid.h" #include "SquidString.h" #include "StrList.h" +#include "base/TextException.h" /** appends an item to the list */ void strListAdd(String * str, const char *item, char del) { assert(str && item); + const String::size_type itemSize = strlen(item); if (str->size()) { char buf[3]; buf[0] = del; buf[1] = ' '; buf[2] = '\0'; + Must(str->canGrowBy(2)); str->append(buf, 2); } - str->append(item, strlen(item)); + Must(str->canGrowBy(itemSize)); + str->append(item, itemSize); } /** returns true iff "m" is a member of the list */ === modified file 'src/String.cc' --- src/String.cc 2016-01-01 00:14:27 +0000 +++ src/String.cc 2016-02-19 23:15:41 +0000 @@ -67,7 +67,7 @@ String::setBuffer(char *aBuf, String::size_type aSize) { assert(undefined()); - assert(aSize < 65536); + assert(aSize <= SizeMax_); buf_ = aBuf; size_ = aSize; } @@ -198,7 +198,7 @@ } else { // Create a temporary string and absorb it later. String snew; - assert(len_ + len < 65536); // otherwise snew.len_ overflows below + assert(canGrowBy(len)); // otherwise snew.len_ may overflow below snew.len_ = len_ + len; snew.allocBuffer(snew.len_ + 1); === modified file 'src/Server.cc' --- src/Server.cc 2016-01-31 05:39:09 +0000 +++ src/Server.cc 2016-02-19 23:15:41 +0000 @@ -68,6 +68,7 @@ startedAdaptation(false), #endif receivedWholeRequestBody(false), + doneWithFwd(NULL), theVirginReply(NULL), theFinalReply(NULL) { @@ -95,8 +96,6 @@ HTTPMSGUNLOCK(theVirginReply); HTTPMSGUNLOCK(theFinalReply); - fwd = NULL; // refcounted - if (responseBodyBuffer != NULL) { delete responseBodyBuffer; responseBodyBuffer = NULL; @@ -114,6 +113,14 @@ cleanAdaptation(); #endif + if (!doneWithServer()) + closeServer(); + + if (!doneWithFwd) { + doneWithFwd = "swanSong()"; + fwd->handleUnregisteredServerEnd(); + } + BodyConsumer::swanSong(); #if USE_ADAPTATION Initiator::swanSong(); @@ -238,6 +246,7 @@ { debugs(11,5, HERE << "completing forwarding for " << fwd); assert(fwd != NULL); + doneWithFwd = "completeForwarding()"; fwd->complete(); } === modified file 'src/Server.h' --- src/Server.h 2016-01-31 05:39:09 +0000 +++ src/Server.h 2016-02-19 23:15:41 +0000 @@ -192,6 +192,10 @@ #endif bool receivedWholeRequestBody; ///< handleRequestBodyProductionEnded called + /// whether we should not be talking to FwdState; XXX: clear fwd instead + /// points to a string literal which is used only for debugging + const char *doneWithFwd; + private: void sendBodyIsTooLargeError(); void maybePurgeOthers(); === modified file 'src/ftp.cc' --- src/ftp.cc 2016-01-31 05:39:09 +0000 +++ src/ftp.cc 2016-02-19 23:15:41 +0000 @@ -445,6 +445,7 @@ { debugs(9, 4, HERE); ctrl.clear(); + doneWithFwd = "ctrlClosed()"; // assume FwdState is monitoring too mustStop("FtpStateData::ctrlClosed"); } @@ -3853,26 +3854,14 @@ return true; } -/** - * Quickly abort the transaction - * - \todo destruction should be sufficient as the destructor should cleanup, - * including canceling close handlers - */ void FtpStateData::abortTransaction(const char *reason) { debugs(9, 3, HERE << "aborting transaction for " << reason << "; FD " << (ctrl.conn!=NULL?ctrl.conn->fd:-1) << ", Data FD " << (data.conn!=NULL?data.conn->fd:-1) << ", this " << this); - if (Comm::IsConnOpen(ctrl.conn)) { - ctrl.conn->close(); - return; - } - - fwd->handleUnregisteredServerEnd(); - mustStop("FtpStateData::abortTransaction"); + mustStop(reason); } /// creates a data channel Comm close callback AsyncCall::Pointer === modified file 'src/http.cc' --- src/http.cc 2016-02-18 04:15:33 +0000 +++ src/http.cc 2016-02-19 23:15:41 +0000 @@ -173,6 +173,7 @@ HttpStateData::httpStateConnClosed(const CommCloseCbParams ¶ms) { debugs(11, 5, "httpStateFree: FD " << params.fd << ", httpState=" << params.data); + doneWithFwd = "httpStateConnClosed()"; // assume FwdState is monitoring too mustStop("HttpStateData::httpStateConnClosed"); } @@ -1757,7 +1758,8 @@ String strFwd = hdr_in->getList(HDR_X_FORWARDED_FOR); - if (strFwd.size() > 65536/2) { + // if we cannot double strFwd size, then it grew past 50% of the limit + if (!strFwd.canGrowBy(strFwd.size())) { // There is probably a forwarding loop with Via detection disabled. // If we do nothing, String will assert on overflow soon. // TODO: Terminate all transactions with huge XFF? @@ -2408,20 +2410,10 @@ ServerStateData::sentRequestBody(io); } -// Quickly abort the transaction -// TODO: destruction should be sufficient as the destructor should cleanup, -// including canceling close handlers void HttpStateData::abortTransaction(const char *reason) { debugs(11,5, HERE << "aborting transaction for " << reason << "; " << serverConnection << ", this " << this); - - if (Comm::IsConnOpen(serverConnection)) { - serverConnection->close(); - return; - } - - fwd->handleUnregisteredServerEnd(); - mustStop("HttpStateData::abortTransaction"); + mustStop(reason); }
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
Attachments on
bug 17816
:
7493
| 7494