Message-ID: <8312d279-688b-4fe7-870e-8243a8f883fa@auristor.com>
Date:   Tue, 30 May 2023 10:39:29 -0400
From:   Jeffrey E Altman <jaltman@...istor.com>
To:     David Laight <David.Laight@...LAB.COM>,
        Kenny Ho <y2kenny@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Marc Dionne <marc.dionne@...istor.com>,
        Kenny Ho <Kenny.Ho@....com>,
        David Howells <dhowells@...hat.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        "linux-afs@...ts.infradead.org" <linux-afs@...ts.infradead.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] Remove hardcoded static string length

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@...lab.com>  wrote:
>>>>> "The standard formulation seems to be: <project> <version> built
>>>>> <yyyy>-<mm>-<dd>"
>>>> 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:
>>>
>>> "linux-" UTS_RELEASE " AF_RXRPC"
>>>
>>> Kenny
>> 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.
Exactly.
>>   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



Download attachment "smime.p7s" of type "application/pkcs7-signature" (4039 bytes)

Powered by blists - more mailing lists