The following patch should handle BIG-REQUESTS. Please review.
---
While developing the patch, I noticed that without BIG-REQUESTS (e.g.
when HIDE_BIG_REQUESTS_EXTENSION is in use), the "bad client" would send
invalid X protocol data that caused nxproxy to crash. This patch also
handles that case, though that does not occur anymore when BIG-REQUESTS
is available. - There may be other places where invalid X protocol data
could crash nxproxy, but I did not change the code.
How can the "bad client" send invalid X protocol (what libraries are
buggy and broken), is an interesting question (but is not this bug).
Cheers, Paul
Paul Szabo [email protected] http://www.maths.usyd.edu.au/u/psz/
School of Mathematics and Statistics University of Sydney Australia
--- ClientChannel.cpp-prePSz 2012-03-08 06:53:30.000000000 +1100
+++ ClientChannel.cpp 2014-10-24 14:20:54.000000000 +1100
@@ -447,6 +447,26 @@
}
}
+ // Get other bits of the header, so will not need to refer to them again
+ unsigned char inputDataByte = inputMessage[1];
+ unsigned int buffer2 = GetUINT(inputMessage + 2, bigEndian_);
+ unsigned int inputDataSize = buffer2 - 1;
+ if (buffer2 == 0)
+ {
+ // BIG-REQUESTS
+ inputMessage += 4;
+ inputLength -= 4;
+ inputDataSize = GetULONG(inputMessage, bigEndian_) - 2;
+ }
+ if (inputLength != (4 * (inputDataSize + 1)))
+ {
+ #ifdef WARNING
+ *logofs << "handleRead: inputLength=" << inputLength
+ << " mismatch inputDataSize=" << inputDataSize
+ << ".\n" << logofs_flush;
+ #endif
+ }
+
//
// Go to the message's specific encoding.
//
@@ -501,8 +521,36 @@
encodeBuffer.encodeCachedValue(format, 8,
clientCache_ -> changePropertyFormatCache);
unsigned int dataLength = GetULONG(inputMessage + 20, bigEndian_);
+
+ // Self-preserving sanity check (otherwise we crash and dump core):
+ // some clients do this when not getting their beloved BIG-REQUESTS.
+ unsigned int maxLength = 0;
+ if (format == 8)
+ {
+ maxLength = inputLength - 24;
+ }
+ else if (format == 32)
+ {
+ maxLength = (inputLength - 24) >> 2;
+ }
+ else if (format == 16)
+ {
+ maxLength = (inputLength - 24) >> 1;
+ }
+ if (dataLength > maxLength)
+ {
+ #ifdef WARNING
+ *logofs << "ChangeProperty bogus dataLength=" << dataLength
+ << " set to " << maxLength
+ << " when format=" << (int)format
+ << " inputLength=" << inputLength
+ << ".\n" << logofs_flush;
+ #endif
+ dataLength = maxLength;
+ }
+
encodeBuffer.encodeValue(dataLength, 32, 6);
- encodeBuffer.encodeValue(inputMessage[1], 2);
+ encodeBuffer.encodeValue(inputDataByte, 2);
encodeBuffer.encodeXidValue(GetULONG(inputMessage + 4, bigEndian_),
clientCache_ -> windowCache);
encodeBuffer.encodeCachedValue(GetULONG(inputMessage + 8, bigEndian_), 29,
@@ -533,7 +581,7 @@
nextSrc += 4;
}
}
- else
+ else if (format == 16)
{
for (unsigned int i = 0; i < dataLength; i++)
{
@@ -541,6 +589,13 @@
nextSrc += 2;
}
}
+ else
+ {
+ #ifdef WARNING
+ *logofs << "ChangeProperty bogus format=" << (int)format
+ << ".\n" << logofs_flush;
+ #endif
+ }
}
break;
case X_SendEvent:
@@ -562,7 +617,7 @@
break;
}
- encodeBuffer.encodeBoolValue((unsigned int) inputMessage[1]);
+ encodeBuffer.encodeBoolValue((unsigned int) inputDataByte);
unsigned int window = GetULONG(inputMessage + 4, bigEndian_);
if (window == 0 || window == 1)
@@ -599,7 +654,7 @@
break;
case X_ChangeWindowAttributes:
{
- encodeBuffer.encodeValue((inputLength - 12) >> 2, 4);
+ encodeBuffer.encodeValue(inputDataSize - 2, 4);
encodeBuffer.encodeXidValue(GetULONG(inputMessage + 4, bigEndian_),
clientCache_ -> windowCache);
unsigned int bitmask = GetULONG(inputMessage + 8, bigEndian_);
@@ -654,7 +709,7 @@
break;
}
- encodeBuffer.encodeBoolValue((unsigned int) inputMessage[1]);
+ encodeBuffer.encodeBoolValue((unsigned int) inputDataByte);
encodeBuffer.encodeXidValue(GetULONG(inputMessage + 4, bigEndian_),
clientCache_ -> windowCache);
const unsigned char *nextSrc = inputMessage + 8;
@@ -1000,12 +1055,12 @@
{
#ifdef TARGETS
- *logofs << "handleRead: X_CreatePixmap depth " << (unsigned) inputMessage[1]
+ *logofs << "handleRead: X_CreatePixmap depth " << (unsigned) inputDataByte
<< ", pixmap id " << GetULONG(inputMessage + 4, bigEndian_)
<< ", drawable " << GetULONG(inputMessage + 8, bigEndian_)
<< ", width " << GetUINT(inputMessage + 12, bigEndian_)
<< ", height " << GetUINT(inputMessage + 14, bigEndian_)
- << ", size " << GetUINT(inputMessage + 2, bigEndian_) << 2
+ << ", length " << inputLength
<< ".\n" << logofs_flush;
unsigned int p_id = GetULONG(inputMessage + 4, bigEndian_);
@@ -1054,7 +1109,7 @@
#endif
unsigned bitmask = GetULONG(inputMessage + 28, bigEndian_);
- encodeBuffer.encodeCachedValue((unsigned int) inputMessage[1], 8,
+ encodeBuffer.encodeCachedValue((unsigned int) inputDataByte, 8,
clientCache_ -> depthCache);
encodeBuffer.encodeXidValue(GetULONG(inputMessage + 8, bigEndian_),
clientCache_ -> windowCache);
@@ -1138,7 +1193,7 @@
break;
}
- unsigned int numPoints = ((inputLength - 16) >> 2);
+ unsigned int numPoints = (inputDataSize - 3);
if (control -> isProtoStep10() == 1)
{
@@ -1209,7 +1264,7 @@
break;
case X_FreeColors:
{
- unsigned int numPixels = GetUINT(inputMessage + 2, bigEndian_) - 3;
+ unsigned int numPixels = inputDataSize - 2;
encodeBuffer.encodeValue(numPixels, 16, 4);
encodeBuffer.encodeCachedValue(GetULONG(inputMessage + 4, bigEndian_), 29,
clientCache_ -> colormapCache);
@@ -1378,7 +1433,7 @@
break;
}
- encodeBuffer.encodeBoolValue((unsigned int) inputMessage[1]);
+ encodeBuffer.encodeBoolValue((unsigned int) inputDataByte);
encodeBuffer.encodeXidValue(GetULONG(inputMessage + 4, bigEndian_),
clientCache_ -> windowCache);
unsigned int property = GetULONG(inputMessage + 8, bigEndian_);
@@ -1404,7 +1459,7 @@
break;
case X_GrabButton:
{
- encodeBuffer.encodeBoolValue((unsigned int) inputMessage[1]);
+ encodeBuffer.encodeBoolValue((unsigned int) inputDataByte);
encodeBuffer.encodeXidValue(GetULONG(inputMessage + 4, bigEndian_),
clientCache_ -> windowCache);
encodeBuffer.encodeCachedValue(GetUINT(inputMessage + 8, bigEndian_), 16,
@@ -1423,7 +1478,7 @@
break;
case X_GrabPointer:
{
- encodeBuffer.encodeBoolValue((unsigned int) inputMessage[1]);
+ encodeBuffer.encodeBoolValue((unsigned int) inputDataByte);
encodeBuffer.encodeXidValue(GetULONG(inputMessage + 4, bigEndian_),
clientCache_ -> windowCache);
encodeBuffer.encodeCachedValue(GetUINT(inputMessage + 8, bigEndian_), 16,
@@ -1448,7 +1503,7 @@
break;
case X_GrabKeyboard:
{
- encodeBuffer.encodeBoolValue((unsigned int) inputMessage[1]);
+ encodeBuffer.encodeBoolValue((unsigned int) inputDataByte);
encodeBuffer.encodeXidValue(GetULONG(inputMessage + 4, bigEndian_),
clientCache_ -> windowCache);
unsigned int timestamp = GetULONG(inputMessage + 8, bigEndian_);
@@ -1673,7 +1728,7 @@
break;
}
- unsigned int textLength = (unsigned int) inputMessage[1];
+ unsigned int textLength = (unsigned int) inputDataByte;
encodeBuffer.encodeCachedValue(textLength, 8,
clientCache_ -> imageTextLengthCache, 4);
encodeBuffer.encodeXidValue(GetULONG(inputMessage + 4,
@@ -1740,7 +1795,7 @@
break;
}
- unsigned int textLength = (unsigned int) inputMessage[1];
+ unsigned int textLength = (unsigned int) inputDataByte;
encodeBuffer.encodeCachedValue(textLength, 8,
clientCache_ -> imageTextLengthCache, 4);
encodeBuffer.encodeXidValue(GetULONG(inputMessage + 4,
@@ -1797,7 +1852,7 @@
unsigned int nameLength = GetUINT(inputMessage + 4, bigEndian_);
encodeBuffer.encodeValue(nameLength, 16, 6);
- encodeBuffer.encodeBoolValue((unsigned int) inputMessage[1]);
+ encodeBuffer.encodeBoolValue((unsigned int) inputDataByte);
const unsigned char *nextSrc = inputMessage + 8;
if (control -> isProtoStep7() == 1)
@@ -2269,8 +2324,8 @@
break;
}
- encodeBuffer.encodeValue(GetUINT(inputMessage + 2, bigEndian_) - 3, 16, 4);
- encodeBuffer.encodeBoolValue((unsigned int) inputMessage[1]);
+ encodeBuffer.encodeValue(inputDataSize - 2, 32, 4);
+ encodeBuffer.encodeBoolValue((unsigned int) inputDataByte);
encodeBuffer.encodeXidValue(GetULONG(inputMessage + 4, bigEndian_),
clientCache_ -> drawableCache);
encodeBuffer.encodeXidValue(GetULONG(inputMessage + 8, bigEndian_),
@@ -2336,8 +2391,8 @@
break;
}
- encodeBuffer.encodeValue(GetUINT(inputMessage + 2, bigEndian_) - 3, 16, 4);
- encodeBuffer.encodeBoolValue((unsigned int) inputMessage[1]);
+ encodeBuffer.encodeValue(inputDataSize - 2, 32, 4);
+ encodeBuffer.encodeBoolValue((unsigned int) inputDataByte);
encodeBuffer.encodeXidValue(GetULONG(inputMessage + 4,
bigEndian_), clientCache_ -> drawableCache);
encodeBuffer.encodeXidValue(GetULONG(inputMessage + 8,
@@ -2370,8 +2425,7 @@
break;
case X_PolyRectangle:
{
- encodeBuffer.encodeValue((GetUINT(inputMessage + 2,
- bigEndian_) - 3) >> 1, 16, 3);
+ encodeBuffer.encodeValue((inputDataSize - 2) >> 1, 32, 3);
encodeBuffer.encodeXidValue(GetULONG(inputMessage + 4,
bigEndian_), clientCache_ -> drawableCache);
encodeBuffer.encodeXidValue(GetULONG(inputMessage + 8,
@@ -2424,8 +2478,7 @@
break;
}
- encodeBuffer.encodeValue((GetUINT(inputMessage + 2,
- bigEndian_) - 3) >> 1, 16, 4);
+ encodeBuffer.encodeValue((inputDataSize - 2) >> 1, 32, 4);
encodeBuffer.encodeXidValue(GetULONG(inputMessage + 4,
bigEndian_), clientCache_ -> drawableCache);
encodeBuffer.encodeXidValue(GetULONG(inputMessage + 8,
@@ -2522,7 +2575,7 @@
break;
case X_QueryBestSize:
{
- encodeBuffer.encodeValue((unsigned int)inputMessage[1], 2);
+ encodeBuffer.encodeValue((unsigned int)inputDataByte, 2);
encodeBuffer.encodeXidValue(GetULONG(inputMessage + 4,
bigEndian_), clientCache_ -> drawableCache);
encodeBuffer.encodeValue(GetUINT(inputMessage + 8, bigEndian_), 16, 8);
@@ -2538,7 +2591,7 @@
// Differential encoding.
encodeBuffer.encodeBoolValue(1);
- unsigned int numColors = ((inputLength - 8) >> 2);
+ unsigned int numColors = (inputDataSize - 1);
encodeBuffer.encodeValue(numColors, 16, 5);
encodeBuffer.encodeCachedValue(GetULONG(inputMessage + 4, bigEndian_), 29,
clientCache_ -> colormapCache);
@@ -2636,7 +2689,7 @@
break;
}
- unsigned int numRectangles = ((inputLength - 12) >> 3);
+ unsigned int numRectangles = ((inputDataSize - 2) >> 1);
if (control -> isProtoStep9() == 1)
{
@@ -2647,7 +2700,7 @@
encodeBuffer.encodeValue(numRectangles, 13, 4);
}
- encodeBuffer.encodeValue((unsigned int) inputMessage[1], 2);
+ encodeBuffer.encodeValue((unsigned int) inputDataByte, 2);
encodeBuffer.encodeXidValue(GetULONG(inputMessage + 4, bigEndian_),
clientCache_ -> gcCache);
encodeBuffer.encodeCachedValue(GetUINT(inputMessage + 8, bigEndian_), 16,
@@ -2802,7 +2855,7 @@
}
// Format.
- encodeBuffer.encodeValue((unsigned int) inputMessage[1], 2);
+ encodeBuffer.encodeValue((unsigned int) inputDataByte, 2);
// Drawable.
encodeBuffer.encodeXidValue(GetULONG(inputMessage + 4,
bigEndian_), clientCache_ -> drawableCache);
@@ -3004,7 +3057,7 @@
<< ".\n" << logofs_flush;
#endif
- encodeBuffer.encodeCachedValue(*(inputMessage + 1), 8,
+ encodeBuffer.encodeCachedValue(inputDataByte, 8,
clientCache_ -> resourceCache);
}
else if (inputOpcode == opcodeStore_ -> freeUnpack)
@@ -3015,7 +3068,7 @@
<< ".\n" << logofs_flush;
#endif
- encodeBuffer.encodeCachedValue(*(inputMessage + 1), 8,
+ encodeBuffer.encodeCachedValue(inputDataByte, 8,
clientCache_ -> resourceCache);
}
else if (inputOpcode == opcodeStore_ -> getControlParameters)
@@ -3198,10 +3251,10 @@
{
if (hit)
{
- statistics -> addRenderCachedRequest(*(inputMessage + 1));
+ statistics -> addRenderCachedRequest(inputDataByte);
}
- statistics -> addRenderRequestBits(*(inputMessage + 1), inputLength << 3, bits);
+ statistics -> addRenderRequestBits(inputDataByte, inputLength << 3, bits);
}
} // End if (firstRequest_)... else ...
--- ClientReadBuffer.cpp-prePSz 2012-03-08 06:53:30.000000000 +1100
+++ ClientReadBuffer.cpp 2014-10-24 12:25:47.000000000 +1100
@@ -119,15 +119,32 @@
dataLength = (GetUINT(start + 2, bigEndian_) << 2);
- if (dataLength < 4)
+ if (dataLength == 0) // or equivalently (dataLength < 4)
{
- #ifdef TEST
- *logofs << "ClientReadBuffer: WARNING! Assuming length 4 "
- << "for suspicious message of length " << dataLength
- << ".\n" << logofs_flush;
- #endif
+ // BIG-REQUESTS extension
+ if (size < 8)
+ {
+ remaining_ = 8 - size;
+ return 0;
+ }
- dataLength = 4;
+ dataLength = (GetULONG(start + 4, bigEndian_) << 2);
+
+ if (dataLength < 8 || dataLength > 1024*1024*1024)
+ {
+ #ifdef WARNING
+ *logofs << "BIG-REQUESTS with unacceptable dataLength="
+ << dataLength << ", now set to 8.\n" << logofs_flush;
+ #endif
+ dataLength = 8;
+ }
+ else if (dataLength < 4*64*1024)
+ {
+ #ifdef WARNING
+ *logofs << "BIG-REQUESTS with silly dataLength="
+ << dataLength << ".\n" << logofs_flush;
+ #endif
+ }
}
}
--- ServerChannel.cpp-prePSz 2012-03-08 06:53:30.000000000 +1100
+++ ServerChannel.cpp 2014-10-24 12:38:04.000000000 +1100
@@ -102,7 +102,8 @@
//
#define HIDE_MIT_SHM_EXTENSION
-#define HIDE_BIG_REQUESTS_EXTENSION
+// HIDE_BIG_REQUESTS_EXTENSION : No good to hide, some clients may send crap instead...
+#undef HIDE_BIG_REQUESTS_EXTENSION
#define HIDE_XFree86_Bigfont_EXTENSION
#undef HIDE_SHAPE_EXTENSION
#undef HIDE_XKEYBOARD_EXTENSION
@@ -3756,7 +3757,7 @@
}
unsigned int numPoints;
- decodeBuffer.decodeValue(numPoints, 16, 4);
+ decodeBuffer.decodeValue(numPoints, 32, 4);
outputLength = (numPoints << 2) + 12;
outputMessage = writeBuffer_.addMessage(outputLength);
unsigned int relativeCoordMode;
@@ -3802,7 +3803,7 @@
}
unsigned int numPoints;
- decodeBuffer.decodeValue(numPoints, 16, 4);
+ decodeBuffer.decodeValue(numPoints, 32, 4);
outputLength = (numPoints << 2) + 12;
outputMessage = writeBuffer_.addMessage(outputLength);
unsigned int relativeCoordMode;
@@ -3839,7 +3840,7 @@
case X_PolyRectangle:
{
unsigned int numRectangles;
- decodeBuffer.decodeValue(numRectangles, 16, 3);
+ decodeBuffer.decodeValue(numRectangles, 32, 3);
outputLength = (numRectangles << 3) + 12;
outputMessage = writeBuffer_.addMessage(outputLength);
decodeBuffer.decodeXidValue(value, clientCache_ -> drawableCache);
@@ -3869,7 +3870,7 @@
}
unsigned int numSegments;
- decodeBuffer.decodeValue(numSegments, 16, 4);
+ decodeBuffer.decodeValue(numSegments, 32, 4);
outputLength = (numSegments << 3) + 12;
outputMessage = writeBuffer_.addMessage(outputLength);
decodeBuffer.decodeXidValue(value, clientCache_ -> drawableCache);
@@ -4590,7 +4591,21 @@
*outputMessage = (unsigned char) outputOpcode;
- PutUINT(outputLength >> 2, outputMessage + 2, bigEndian_);
+ if (outputLength < 4*64*1024)
+ PutUINT(outputLength >> 2, outputMessage + 2, bigEndian_);
+ else
+ {
+ // Handle BIG-REQUESTS
+ PutUINT(0, outputMessage + 2, bigEndian_);
+// BUG ALERT: following write may not work well,
+// particularly with un-flushed messages.
+// But, it works well enough in my testing...
+ // Write first four bytes
+ if (transport_ -> write(write_immediate, outputMessage, 4) < 0)
+ return -1;
+ // Replace with new 4-byte length
+ PutULONG(1 + (outputLength >> 2), outputMessage, bigEndian_);
+ }
#if defined(TEST) || defined(OPCODES)
*logofs << "handleWrite: Handled request OPCODE#"
