MantisBT - Doomseeker
View Issue Details
IDProjectCategoryView StatusDate SubmittedLast Update
0003660Doomseeker[All Projects] Securitypublic2019-06-12 00:412019-07-31 15:19
ReporterPol M 
Assigned ToPol M 
PrioritynormalSeveritycrashReproducibilityhave not tried
StatusclosedResolutionfixed 
PlatformOSOpenBSDOS Version
Product Version1.2 
Target VersionFixed in Version1.3 
Summary0003660: SRB2 plugin: Mangled memory values (runtime crashes)
Description0003499:0020738:

Quote from Pol M
I'm cross-posting relevant information related to SRB2, this comes from this pr:

Well, working on OpenBSD without the srb2 thing is a pain because 1/5 of the tries end up with a crash caused by srb2 (heck, pretty much all runtime crashes come from that, except for the exit crash). I decided to check for sizes across multiple platforms, and all of them have a header size of 84, and the final header has a size of 0. For some reason, in OpenBSD it has random gigantic sizes (1919905893, 1802265971, 1297293506, 1802265971 for example (in decimal)). All the instances encountered were over 0x0FFFFFFF with plenty of room, and while the header size may vary in the future, it won’t even get close to that.

The rest of the values (id, type, room) also get mangled: 0x35303239, 0x00000000 and 0x875b5553 vs the expected 0x32330000, 0x10000000, 0x000000ce, but I can’t tell how useful this info is.

[...]

I’ve been able to compile and run SRB2 without problems, and I can play online games. So, while probably OpenBSD is not supported, it can be used to play SRB2.
Steps To Reproduce
Additional Information
TagsNo tags attached.
Relationships
parent of 0003665closed  Missing CVE Identifier for issue 0003660 (remote denial of service in SRB2 engine plugin) 
related to 0003499assigned Pol M Port Doomseeker to OpenBSD 
Attached Files
Issue History
Date ModifiedUsernameFieldChange
2019-06-12 00:41WubTheCaptainNew Issue
2019-06-12 00:41WubTheCaptainReporterWubTheCaptain => Pol M
2019-06-12 00:42WubTheCaptainStatusnew => acknowledged
2019-06-12 00:43WubTheCaptainRelationship addedrelated to 0003499
2019-06-12 20:27Pol MNote Added: 0020755
2019-06-12 20:27Pol MAssigned To => Pol M
2019-06-12 20:27Pol MStatusacknowledged => needs testing
2019-06-13 13:24WubTheCaptainTarget Version => 1.3
2019-06-13 13:29WubTheCaptainPrioritylow => normal
2019-06-13 13:29WubTheCaptainSeverityminor => crash
2019-06-13 14:51ZalewaNote Added: 0020756
2019-06-13 17:23Pol MNote Added: 0020762
2019-06-14 04:00WubTheCaptainAssigned ToPol M => Zalewa
2019-06-14 04:00WubTheCaptainStatusneeds testing => needs review
2019-06-14 04:13WubTheCaptainNote Added: 0020764
2019-06-14 04:14WubTheCaptainNote Edited: 0020764bug_revision_view_page.php?bugnote_id=20764#r12633
2019-06-14 04:16WubTheCaptainNote Added: 0020765
2019-06-18 23:56WubTheCaptainNote Added: 0020769
2019-06-18 23:56WubTheCaptainPrioritynormal => high
2019-06-21 14:56ZalewaNote Added: 0020790
2019-06-21 14:56ZalewaStatusneeds review => needs testing
2019-06-21 14:56ZalewaNote Edited: 0020790bug_revision_view_page.php?bugnote_id=20790#r12657
2019-06-21 15:05Pol MNote Added: 0020792
2019-06-21 15:06ZalewaNote Added: 0020793
2019-06-21 21:11WubTheCaptainRelationship addedparent of 0003665
2019-06-21 21:13WubTheCaptainNote Added: 0020795
2019-06-21 21:16WubTheCaptainCategoryBug => Security
2019-06-21 22:43WubTheCaptainPriorityhigh => normal
2019-06-22 10:47ZalewaAssigned ToZalewa => Pol M
2019-06-22 10:47ZalewaStatusneeds testing => assigned
2019-06-22 10:47ZalewaStatusassigned => needs testing
2019-06-28 08:27WubTheCaptainNote Added: 0020842
2019-06-28 08:27WubTheCaptainStatusneeds testing => feedback
2019-07-20 06:32ZalewaTarget Version1.3 =>
2019-07-30 10:51WubTheCaptainNote Added: 0020945
2019-07-30 10:52WubTheCaptainNote Edited: 0020945bug_revision_view_page.php?bugnote_id=20945#r12760
2019-07-30 11:13Pol MNote Added: 0020948
2019-07-30 11:13Pol MStatusfeedback => assigned
2019-07-31 15:15Pol MNote Edited: 0020948bug_revision_view_page.php?bugnote_id=20948#r12765
2019-07-31 15:18Pol MNote Edited: 0020948bug_revision_view_page.php?bugnote_id=20948#r12766
2019-07-31 15:19Pol MNote Added: 0020957
2019-07-31 15:19Pol MStatusassigned => closed
2019-07-31 15:19Pol MResolutionopen => fixed
2019-07-31 15:19Pol MFixed in Version => 1.3
2019-07-31 15:19Pol MNote Edited: 0020948bug_revision_view_page.php?bugnote_id=20948#r12767

Notes
(0020755)
Pol M   
2019-06-12 20:27   
This has been addressed. commit
Marking as needs testing
(0020756)
Zalewa   
2019-06-13 14:51   
This is still just a quick fix that will work most of the time but not at all times. When programming the plugin I've never assumed that the master server will be malicious - while this is not the case here, it essentially is from Doomseeker's perspective as the packets arrive mangled for some reason. The code should be more defensive to prevent malicious packets from crashing the program - always.

What will happen if the header.length is below the current threshold but still incorrect? This could be easily simulated even on a non-BSD system by hardcoding header.length = 0xffffff.
(0020762)
Pol M   
2019-06-13 17:23   
Now that I think about it, we know the size based on the standard. It's:
[code=src/plugins/srb2/srb2masterclient.cpp:138]
QDataStream &operator>>(QDataStream &stream, ServerPayload &server)
{
    DataStreamOperatorWrapper out = DataStreamOperatorWrapper(&stream);

    out.readRaw(16); // Header full of zeros.
    server.rawIp(out.readRaw(16));
    server.rawPort(out.readRaw(8));
    server.name = Srb2::asciiOnly(out.readRaw(32));
    server.room = out.readQInt32();
    server.version = out.readRaw(8);

    return stream;
};

so that'd be 16+16+8+32+4+8=84 #quick maths.
We could define a static and constant value in the ServerPayload class.
(0020764)
WubTheCaptain   
2019-06-14 04:13   
(edited on: 2019-06-14 04:14)
Quote from Zalewa
When programming the plugin I've never assumed that the master server will be malicious - while this is not the case here, it essentially is from Doomseeker's perspective as the packets arrive mangled for some reason. The code should be more defensive to prevent malicious packets from crashing the program - always.


If you know this to be an issue for other plugins, please report new issue(s) (for every affected plugin). Thanks.
(0020765)
WubTheCaptain   
2019-06-14 04:16   
On the note that this is remote denial of service, do we need to address this as a security issue with a CVE Identifier?
(0020769)
WubTheCaptain   
2019-06-18 23:56   
(Increasing issue priority per comment 0003660:0020765 above.)
(0020790)
Zalewa   
2019-06-21 14:56   
The PR is merged here:'https://bitbucket.org/Doomseeker/doomseeker/commits/b9a90f1f56e704c5cbeefe83da2f9ce939920278 [^]'

SRB2 servers are still being listed correctly (and SRB2Kart incorrectly) on Windows so I assume it works, however I'll put it into "needs to be tested" state just in case if someone wants to verify on the BSD platform where the problems were first encountered.

As far as CVE goes - that's bureaucracy and I don't feel like doing it.

Mangled packet problems that may or may not exist in other areas of the program are out of scope for this issue.
(0020792)
Pol M   
2019-06-21 15:05   
Just to make this clear, this issue has nothing to do with the srb2kart player packages, the srb2kart playerlist is still brocken. It's an issue on their, it will be fixed in the next release and has already been patched.
(0020793)
Zalewa   
2019-06-21 15:06   
Of course, by stating that SRB2Kart still works incorrectly I meant that this is expected. :)
(0020795)
WubTheCaptain   
2019-06-21 21:13   
Quote from Zalewa
As far as CVE goes - that's bureaucracy and I don't feel like doing it.


I feel like giving it a try. Tracked as 0003665.

It would be preferable to have a version bump for the SRB2 engine plugin, I'd guess.
(0020842)
WubTheCaptain   
2019-06-28 08:27   
Can you reproduce the issue in OP with compiler optimizations turned off (-O0 flag)? (I believe CMake's debug builds also disable optimizations.)
(0020945)
WubTheCaptain   
2019-07-30 10:51   
(edited on: 2019-07-30 10:52)
FYI, this was mentioned to be "fixed" in Doomseeker 1.3 changelogs.

Quote
SRB2: Discard master server header replies that don't have the expected length declaration, thus preventing crashes when trying to read them. (0003660)
(0020948)
Pol M   
2019-07-30 11:13   
(edited on: 2019-07-31 15:19)
It is resolved, the reason this is open is because you asked for confirmation under a desired enviorment, which I cannot provide due to OpenBSD all of a sudden crashing Doomseeker at start since 6.5. I'll eventually find a workaround, I'm suspicious of the qt5 library.

EDIT: Turns out it was pledge all along. I'll be adding even more permissions.
EDIT2: Confirmed to work, no crashes from the plugin under unnatural header sizes.
(0020957)
Pol M   
2019-07-31 15:19   
Moving directly to closed due to this being part of the 1.3 release.