Ticket #583 (Fixed)Thu Jan 19 20:25:41 UTC 2023
MessageTrans_EnumerateTokens fails when buffer overruns: does not seem to respect length parameter
| Reported by: | Phil Pemberton (7989) | Severity: | Major |
| Part: | RISC OS: Module | Release: | |
| Milestone: | Status | Fixed |
Details by Phil Pemberton (7989):
I’ve been working on the PortManager (PortMan) module, with the intent of running RISC OS on the Bush IBX100 STB. (I currently have a working, but old Kernel).
I’ve hit an issue with MessageTrans_EnumerateTokens.
The PortMan “c.tags” file contains a function, tag_init, which loads the tag file.
If the length of a tag in the tag file is greater than the buffer size, MessageTrans_EnumerateTokens will fail and crash the module.
I’ve seen this manifest as data aborts or undefined instruction errors in the SharedCLibrary.
By default TOKEN_MAX_SIZE is 16, which creates a local buffer (“name”) of 16 bytes:
char name[TOKEN_MAX_SIZE]; err = swix (MessageTransEnumerateTokens, _INR(0, 4) | _OUTR(2, 4), &tagsfile.buf0, “*”, &name0, TOKEN_MAX_SIZE, index, &more, &length, &index);With the following tag file, the crash occurs when the second-to-last line is processed:
EEPROM_Protect: 1:0:2
LED_Left: 3:0:3
NTSC: 4:0:0
Front_Panel_Button: 6:0:0
LED_Right: 7:0:3
I’ve tried reducing the buffer length passed to EnumerateTokens, but the same error occurs. It appears that the SWI is either ignoring the length parameter, or incorrectly passing the error back to the caller.
If I change TOKEN_MAX_SIZE to 32, the tag file loads correctly with no errors.
Changelog:
Modified by Julie Stamp (8365) Fri, January 20 2023 - 17:11:52 GMT
Which version of MessageTrans is running please?
I ran over that file just in BASIC with a buffer size of 16 and MessageTrans_EnumerateTokens responded ‘Buffer Overflow’ at exactly the right place.
Modified by Julie Stamp (8365) Fri, January 20 2023 - 17:21:38 GMT
I used MessageTrans 0.49 (26 May 2012)
Modified by Phil Pemberton (7989) Fri, January 20 2023 - 21:31:00 GMT
- Status changed from Open to Fixed
Just checked – it’s 0.31.
Looks like this was fixed in 0.40:
commit 726e595af01e66631fa9fa99f6067453c3f9e66e (tag: MsgTrans-0_40)
Author: Stewart Brodie <sbrodie@gitlab.riscosopen.org>
Date: Mon Aug 7 13:58:47 2000 +0000
So the fix is to update MessageTrans. D’oh!
Sorry for the confusion!
