The mds_value_remote_expression() function fails with buffer overflow errors

[mwinkel-dev]

Affiliation
DIII-D (submitted on behalf of KeithE)

Version(s) Affected
Client = alpha-7-139-59 and stable-7-153-3
Server = GA's branch

Platform(s)
Client = RHEL 8.10
Server = RHEL 8.10

Installation Method(s)
Client = RPMs from mdsplus.org
Server = build from source?

Describe the bug
A client computer is running a C program that uses MdsValue() to read 11 single-precision IEEE floats from a tree stored on an MDSplus archive server. The C program often causes TDI to crash, including triggering the TDI-E-BOMB error. The C program was analyzed by an "address sanitizer" which detected buffer overflows in the mds_value_remote_expression() function. The problem only occurs with the IEEE floats; it works reliably if only reading ints from the tree.

To Reproduce
See posts below.

Expected behavior
Reading floats from an MDSplus archive server via mdsip should not cause TDI to crash.

Screenshots
n/a

Additional context
The address sanitizer output was sent via email, but is quite lengthy. When this issue is reproduced on a development system, more details will be provided.

[mwinkel-dev]

The problem only arises with thin-client access of a remote tree when using data types that are not allowed.

The C program was calling MdsValue() with DTYPE_FS which is not a valid type for mdsip. The valid types are in include/ipdesc.h. Thus, the C program should have instead been using DTYPE_FLOAT. (When mdsip processes the descriptor, it translates DTYPE_FLOAT into the appropriate data type for the client CPU's architecture, which in this case is IEEE single-precision floating point).

The longer-term fix is to make the mds_value_remote_expression() function more robust so that it detects invalid types.

The function fails because of the following:

• The function uses malloc() to create a buffer to hold the string expression (typically a node name), but does not zero out the buffer after creating it. Thus the buffer will have random junk.
• When an invalid type is passed in, the code skips the switch() that initializes the buffer using a strcpy().
• Instead, control flow falls to the bottom which immediately does a strcat() to a buffer full of junk. The strcat() of course scans the buffer looking for a null terminating character -- which it will often find near the end of the buffer or beyond the buffer. Thus the strcat() can overflow the buffer and/or trash adjacent memory.
• If strcat() doesn't overflow the buffer, then the resulting expression will often start with junk. When that junk expression is sent to the server, it causes the server's TDI interpreter to complain about bad syntax or to crash.
• Occasionally, the malloc() creates a buffer that starts with a zero. In which case, the expression sent to the server will usually run A-OK.

The fix of course is straightforward:

• Right after the malloc is done, the buffer's elements should be set to zero.
• Furthermore, the function should detect impermissible data types and handle them in some robust manner.

[mwinkel-dev]

Experiments with the debugger show that mds_value_remote_expression() is only called by thin-client.

Furthermore, changing the function's malloc() to a calloc() eliminates the buffer overflow problem.

However, the include/dtypedef.h file includes far more data types than are in include/ipdesc.h. It is unclear at this time if all of the dtypedef.h types should be accepted by mds_value_remote_expression(). Perhaps many of them should be blocked (i.e., cause the function to return an error status)?

[mwinkel-dev]

Note that DTYPE_FLOAT is defined differently in include/mdsdescrip.h than in include/ipdescrip.h. There are likely a few other data types that also have multiple definitions.

Regardless, the comments in mds_value_remote_expression() state that the function was designed to work with the ipdescrip.h definitions.