Zandronum Chat on our Discord Server Get the latest version: 3.1
Source Code
My View | View Issues | Change Log | Roadmap | Doomseeker Issue Support Ranking | Rules | My Account
  

View Issue Details [ Jump to Notes ] [ Issue History ] [ Print ]
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.
Attached Files

- Relationships
parent of 0003665closed Missing CVE Identifier for issue 0003660 (remote denial of service in SRB2 engine plugin) 
related to 0003499assignedPol M Port Doomseeker to OpenBSD 

-  Notes
User avatar (0020755)
Pol M (developer)
2019-06-12 20:27

 This has been addressed. commit
Marking as needs testing
User avatar (0020756)
Zalewa (developer)
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.
User avatar (0020762)
Pol M (developer)
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.
User avatar (0020764)
WubTheCaptain (reporter)
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.
User avatar (0020765)
WubTheCaptain (reporter)
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?
User avatar (0020769)
WubTheCaptain (reporter)
2019-06-18 23:56

 (Increasing issue priority per comment 0003660:0020765 above.)
User avatar (0020790)
Zalewa (developer)
2019-06-21 14:56
edited on: 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.
User avatar (0020792)
Pol M (developer)
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.
User avatar (0020793)
Zalewa (developer)
2019-06-21 15:06

 Of course, by stating that SRB2Kart still works incorrectly I meant that this is expected. :)
User avatar (0020795)
WubTheCaptain (reporter)
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.
User avatar (0020842)
WubTheCaptain (reporter)
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.)
User avatar (0020945)
WubTheCaptain (reporter)
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)
User avatar (0020948)
Pol M (developer)
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.
User avatar (0020957)
Pol M (developer)
2019-07-31 15:19

 Moving directly to closed due to this being part of the 1.3 release.

Issue Community Support
This issue is already marked as resolved.
If you feel that is not the case, please reopen it and explain why.
Supporters: No one explicitly supports this issue yet.
Opponents: No one explicitly opposes this issue yet.

- Issue History
Date Modified Username Field Change
2019-06-12 00:41 WubTheCaptain New Issue
2019-06-12 00:41 WubTheCaptain Reporter WubTheCaptain => Pol M
2019-06-12 00:42 WubTheCaptain Status new => acknowledged
2019-06-12 00:43 WubTheCaptain Relationship added related to 0003499
2019-06-12 20:27 Pol M Note Added: 0020755
2019-06-12 20:27 Pol M Assigned To => Pol M
2019-06-12 20:27 Pol M Status acknowledged => needs testing
2019-06-13 13:24 WubTheCaptain Target Version => 1.3
2019-06-13 13:29 WubTheCaptain Priority low => normal
2019-06-13 13:29 WubTheCaptain Severity minor => crash
2019-06-13 14:51 Zalewa Note Added: 0020756
2019-06-13 17:23 Pol M Note Added: 0020762
2019-06-14 04:00 WubTheCaptain Assigned To Pol M => Zalewa
2019-06-14 04:00 WubTheCaptain Status needs testing => needs review
2019-06-14 04:13 WubTheCaptain Note Added: 0020764
2019-06-14 04:14 WubTheCaptain Note Edited: 0020764 View Revisions
2019-06-14 04:16 WubTheCaptain Note Added: 0020765
2019-06-18 23:56 WubTheCaptain Note Added: 0020769
2019-06-18 23:56 WubTheCaptain Priority normal => high
2019-06-21 14:56 Zalewa Note Added: 0020790
2019-06-21 14:56 Zalewa Status needs review => needs testing
2019-06-21 14:56 Zalewa Note Edited: 0020790 View Revisions
2019-06-21 15:05 Pol M Note Added: 0020792
2019-06-21 15:06 Zalewa Note Added: 0020793
2019-06-21 21:11 WubTheCaptain Relationship added parent of 0003665
2019-06-21 21:13 WubTheCaptain Note Added: 0020795
2019-06-21 21:16 WubTheCaptain Category Bug => Security
2019-06-21 22:43 WubTheCaptain Priority high => normal
2019-06-22 10:47 Zalewa Assigned To Zalewa => Pol M
2019-06-22 10:47 Zalewa Status needs testing => assigned
2019-06-22 10:47 Zalewa Status assigned => needs testing
2019-06-28 08:27 WubTheCaptain Note Added: 0020842
2019-06-28 08:27 WubTheCaptain Status needs testing => feedback
2019-07-20 06:32 Zalewa Target Version 1.3 =>
2019-07-30 10:51 WubTheCaptain Note Added: 0020945
2019-07-30 10:52 WubTheCaptain Note Edited: 0020945 View Revisions
2019-07-30 11:13 Pol M Note Added: 0020948
2019-07-30 11:13 Pol M Status feedback => assigned
2019-07-31 15:15 Pol M Note Edited: 0020948 View Revisions
2019-07-31 15:18 Pol M Note Edited: 0020948 View Revisions
2019-07-31 15:19 Pol M Note Added: 0020957
2019-07-31 15:19 Pol M Status assigned => closed
2019-07-31 15:19 Pol M Resolution open => fixed
2019-07-31 15:19 Pol M Fixed in Version => 1.3
2019-07-31 15:19 Pol M Note Edited: 0020948 View Revisions




Questions or other issues? Contact Us.

Links

Copyright © 2000 - 2024 MantisBT Team
Powered by Mantis Bugtracker