From 9801c5efcf8c1774d9c807ebd5d27ac7049ad993 Mon Sep 17 00:00:00 2001 From: Michal Srb Date: Wed, 29 Mar 2017 17:00:30 +0300 Subject: Fix checkNoWait logic in SSecurityPlain. Currently it proceeds only if there aren't enough data in queue and then it blocks waiting. Also the required amount to receive from network is (ulen + plen), not (ulen + plen + 2). This allowed not authenticated clients to deny service to everyone. --- common/rfb/SSecurityPlain.cxx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/rfb/SSecurityPlain.cxx b/common/rfb/SSecurityPlain.cxx index f5a5cc73..05315490 100644 --- a/common/rfb/SSecurityPlain.cxx +++ b/common/rfb/SSecurityPlain.cxx @@ -92,7 +92,7 @@ bool SSecurityPlain::processMsg(SConnection* sc) } if (state == 1) { - if (is->checkNoWait(ulen + plen + 2)) + if (!is->checkNoWait(ulen + plen)) return false; state = 2; pw = new char[plen + 1]; -- cgit v1.2.3 From 62197c89e98be47a174074e4c7429c57767a4929 Mon Sep 17 00:00:00 2001 From: Michal Srb Date: Wed, 29 Mar 2017 17:05:45 +0300 Subject: Limit max username/password size in SSecurityPlain. Setting the limit to 1024 which should be still more than enough. Unlimited ulen and plen can cause various security problems: * Overflow in `is->checkNoWait(ulen + plen)` causing it to contine when there is not enough data and then wait forever. * Overflow in `new char[plen + 1]` that would allocate zero sized array which succeeds but returns pointer that should not be written into. * Allocation failure in `new char[plen + 1]` from trying to allocate too much and crashing the whole server. All those issues can be triggered by a client before authentication. --- common/rfb/SSecurityPlain.cxx | 7 +++++++ common/rfb/SSecurityPlain.h | 3 +++ 2 files changed, 10 insertions(+) diff --git a/common/rfb/SSecurityPlain.cxx b/common/rfb/SSecurityPlain.cxx index 05315490..fc9dff23 100644 --- a/common/rfb/SSecurityPlain.cxx +++ b/common/rfb/SSecurityPlain.cxx @@ -86,8 +86,15 @@ bool SSecurityPlain::processMsg(SConnection* sc) if (state == 0) { if (!is->checkNoWait(8)) return false; + ulen = is->readU32(); + if (ulen > MaxSaneUsernameLength) + throw AuthFailureException("Too long username"); + plen = is->readU32(); + if (plen > MaxSanePasswordLength) + throw AuthFailureException("Too long password"); + state = 1; } diff --git a/common/rfb/SSecurityPlain.h b/common/rfb/SSecurityPlain.h index 080fcd59..2c08c24e 100644 --- a/common/rfb/SSecurityPlain.h +++ b/common/rfb/SSecurityPlain.h @@ -54,6 +54,9 @@ namespace rfb { PasswordValidator* valid; unsigned int ulen, plen, state; CharArray username; + + static const unsigned int MaxSaneUsernameLength = 1024; + static const unsigned int MaxSanePasswordLength = 1024; }; } -- cgit v1.2.3