Re: [PATCH] Remove hardcoded static string length

From: Jeffrey E Altman
Date: Tue May 30 2023 - 10:40:00 EST

On 5/29/2023 9:32 AM, David Laight wrote:
From: Jeffrey E Altman
Sent: 27 May 2023 16:09

On 5/25/2023 11:37 AM, Kenny Ho wrote:
On Thu, May 25, 2023 at 11:04 AM David Laight<David.Laight@xxxxxxxxxx> wrote:
"The standard formulation seems to be: <project> <version> built
Which I don't recall the string actually matching?
Also the people who like reproducible builds don't like __DATE__.
That's correct, it was not matching even when it was introduced. I am
simply taking that as people caring about the content and not simply
making rxrpc_version_string == UTS_RELEASE. The current format is:


The RX_PACKET_TYPE_VERSION query is issued by the "rxdebug <host> <port>
-version" command which prints the received string to stdout.   It has
also been used some implementations to record the version of the peer.
Although it is required that a response to the RX_PACKET_TYPE_VERSION
query be issued, there is no requirement that the returned string
contain anything beyond a single NUL octet.
Does that mean that the zero-padding/truncation to 65 bytes is bogus?

Its bogus.  The original code implemented in 1988 was very sloppy.  Over the years some of the sloppiness was misinterpreted as well thought out design decisions.

The CMU/Transarc/IBM rxdebug allocates a char[64] for the received version c-string.   The original version wrote a 65 octets of a static productVersion c-string that was defined far away from the code that wrote it to the network.   Over time, the productVersion c-string was changed from a fixed length c-string to a build time generated c-string that had variable length.  It could be shorter than 65-octets or longer (up to 2000 octets). However, the Rx code that wrote the version data to the wire continued to write 65-octets regardless of the size of the productVersion c-string.   This was noticed after IBM AFS 3.6 was forked to form OpenAFS because the version strings became shorter.

OpenAFS 1.2 began the practice of constructing the responseData as follows:

1. Allocate a fixed size char[66] on the stack which I will call 'responseData'

2. bzero responseData

3. copy the productVersion into responseData as follows
    snprintf(responseData, sizeof(responseData), "%s", productVersion)

4. write 65 octets from responseData to the wire

This change (OpenAFS commit 902055cc97a8dd26a26af55778c0b3843de3cc70) addressed the problem of productVersion being shorter than 65-octets by writing a padded response but it did nothing to ensure that the response written to the network was NUL terminated if strlen(productVersion) >= 65.

The author of this commit also wrote the rx-spec.txt document that David Howells used as his source.

Its all bogus.  There is absolutely no requirement that a NUL padded buffer be sent.   The response can be a valid c-string of any length with the only constraint being that the resulting udp packet should not be too large to deliver.

Additionally is the response supposed to the '\0' terminated?
The existing code doesn't guarantee that at all.

The IBM/Transarc/CMU derived RX stacks issue process the Get Version request as follows:

1. Allocate a versionBuffer on the stack to store the returned string.  In the original CMU/Transarc/IBM Rx stack this was char[64].

2. Issue RX_PACKET_TYPE_VERSION query which consists of a 28 octet Rx header with no data.

3. Read RX_PACKET_TYPE_VERSION response into a 1500 octet buffer and remember bytesRead

3a. If  bytesRead is less than 28 octets, its too small to be a Rx header, ignore it.

3b. If the Rx header contents do not match the query, discard it.

3c. The data response begins at &buffer[28] and will be (bytesRead - 28) octets.   Copy MIN(sizeof(versionBuffer), (bytesRead - 28)) octets of the responseData to the versionBuffer.

4. NUL terminate the versionBuffer.  Note: prior to OpenAFS 1.6.23 this step was not performed which is why the sender should always NUL terminate the transmitted version data but this is a bug in the receiver and it is not required that the RX_PACKET_TYPE_VERSION response data be limited to a 64 octet c-string.

5. Do something with the contents of the versionBuffer such as
    printf("AFS version: %s\n", versionBuffer);

Although it is convenient to be able to remotely identify the version of
an Rx implementation, there are good reasons why this information should
not be exposed to an anonymous requester:

1. Linux AF_RXRPC is part of the kernel.  As such, returning
UTS_RELEASE identifies to potential attackers the explicit kernel
version, architecture and perhaps distro.  As this query can be
issued anonymously, this provides an information disclosure that can
be used to target known vulnerabilities in the kernel.
I guess it could even be used as a probe to find more/interesting
systems to attack once inside the firewall.
2. The RX_PACKET_TYPE_VERSION reply is larger than the query by the
number of octets in the version data.  As the query is received via
udp with no reachability test, it means that the
RX_PACKET_TYPE_VERSION query/response can be used to perform an 3.3x
amplification attack: 28 octets in and potentially 93 octets out.

With my security hat on I would suggest that either AF_RXRPC return a
single NUL octet or the c-string "AF_RXRPC" and nothing more.
Is there any point including "AF_RXRPC"?
It is almost certainly implied by the message format.

There is no required message format.   IBM AFS could be built such that the resulting version c-string was

  "@(#)CML not accessible: No version information"

which was shorter than 65 octets.

Or the exact text from the standard - which might be:
"version string - to be supplied by O.E.M."
(I've seen hardware versions with strings like the above that
exactly match the datasheet....)

The rx-spec.txt was an attempt by an individual circa 2001 to document the RxRPC protocol from reading the OpenAFS source code. The document did not undergo peer review and the author did not have the benefit of access to the development history or experience developing/maintaining/extending an Rx implementation.   It was an excellent first effort but it should not be considered gospel.

Limiting the version to (eg) 6.2 would give a hint to the
capabilities/bugs without giving away all the relative addresses
in something like a RHEL kernel.

I would be fine with something like "Linux 6.2".

Jeffrey Altman

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature