[PATCH] Remove hardcoded static string length

[PATCH] Remove hardcoded static string length

[Kenny Ho]

UTS_RELEASE length can exceed the hardcoded length.  This is causing
compile error when WERROR is turned on.

Signed-off-by: Kenny Ho <Kenny.Ho@amd.com>
---
 net/rxrpc/local_event.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/rxrpc/local_event.c b/net/rxrpc/local_event.c
index 19e929c7c38b..61d53ee10784 100644
--- a/net/rxrpc/local_event.c
+++ b/net/rxrpc/local_event.c
@@ -16,7 +16,7 @@
 #include <generated/utsrelease.h>
 #include "ar-internal.h"
 
-static const char rxrpc_version_string[65] = "linux-" UTS_RELEASE " AF_RXRPC";
+static const char rxrpc_version_string[] = "linux-" UTS_RELEASE " AF_RXRPC";
 
 /*
  * Reply to a version request
-- 
2.25.1

Re: [PATCH] Remove hardcoded static string length

[Andrew Lunn]

On Tue, May 23, 2023 at 06:39:44PM -0400, Kenny Ho wrote:
> UTS_RELEASE length can exceed the hardcoded length.  This is causing
> compile error when WERROR is turned on.
> 
> Signed-off-by: Kenny Ho <Kenny.Ho@amd.com>
> ---
>  net/rxrpc/local_event.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/rxrpc/local_event.c b/net/rxrpc/local_event.c
> index 19e929c7c38b..61d53ee10784 100644
> --- a/net/rxrpc/local_event.c
> +++ b/net/rxrpc/local_event.c
> @@ -16,7 +16,7 @@
>  #include <generated/utsrelease.h>
>  #include "ar-internal.h"
>  
> -static const char rxrpc_version_string[65] = "linux-" UTS_RELEASE " AF_RXRPC";
> +static const char rxrpc_version_string[] = "linux-" UTS_RELEASE " AF_RXRPC";

This is not an area of the network stack i know about, so please
excuse what might be a dumb question.

How is the protocol defined here? Is there an RFC or some other sort
of standard?

A message is being built and sent over a socket. The size of that
message was fixed, at 65 + sizeof(whdr). Now the message is variable
length. Does the protocol specification actually allow this?

	Andrew

Re: [PATCH] Remove hardcoded static string length

[Marc Dionne]

On Tue, May 23, 2023 at 9:50 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Tue, May 23, 2023 at 06:39:44PM -0400, Kenny Ho wrote:
> > UTS_RELEASE length can exceed the hardcoded length.  This is causing
> > compile error when WERROR is turned on.
> >
> > Signed-off-by: Kenny Ho <Kenny.Ho@amd.com>
> > ---
> >  net/rxrpc/local_event.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/rxrpc/local_event.c b/net/rxrpc/local_event.c
> > index 19e929c7c38b..61d53ee10784 100644
> > --- a/net/rxrpc/local_event.c
> > +++ b/net/rxrpc/local_event.c
> > @@ -16,7 +16,7 @@
> >  #include <generated/utsrelease.h>
> >  #include "ar-internal.h"
> >
> > -static const char rxrpc_version_string[65] = "linux-" UTS_RELEASE " AF_RXRPC";
> > +static const char rxrpc_version_string[] = "linux-" UTS_RELEASE " AF_RXRPC";
>
> This is not an area of the network stack i know about, so please
> excuse what might be a dumb question.
>
> How is the protocol defined here? Is there an RFC or some other sort
> of standard?
>
> A message is being built and sent over a socket. The size of that
> message was fixed, at 65 + sizeof(whdr). Now the message is variable
> length. Does the protocol specification actually allow this?
>
>         Andrew

I don't think there is an RFC describing RX, but the closest thing to
a spec (https://web.mit.edu/kolya/afs/rx/rx-spec) states:

"If a server receives a packet with a type value of 13, and the
client-initiated flag set, it should respond with a 65-byte payload
containing a string that identifies the version of AFS software it is
running."

So while it may not actually cause any issues (the few things that
look at the data just truncate past 65), it's probably best to keep
the response at a fixed 65 bytes.

Marc

Re: [PATCH] Remove hardcoded static string length

[Andrew Lunn]

> I don't think there is an RFC describing RX, but the closest thing to
> a spec (https://web.mit.edu/kolya/afs/rx/rx-spec) states:
> 
> "If a server receives a packet with a type value of 13, and the
> client-initiated flag set, it should respond with a 65-byte payload
> containing a string that identifies the version of AFS software it is
> running."
> 
> So while it may not actually cause any issues (the few things that
> look at the data just truncate past 65), it's probably best to keep
> the response at a fixed 65 bytes.

Thanks for the link and the quote.

So the compiler warning/error needs to be fixed a different want.

    Andrew

---
pw-bot: cr

Re: [PATCH] Remove hardcoded static string length

[Kenny Ho]

On Wed, May 24, 2023 at 12:02 PM Andrew Lunn <andrew@lunn.ch> wrote:
> So the compiler warning/error needs to be fixed a different want.

Understood.  Would caping the length at iov_len with a ternary be sufficient?

Regards,
Kenny

Re: [PATCH] Remove hardcoded static string length

[Andrew Lunn]

On Wed, May 24, 2023 at 01:02:36PM -0400, Kenny Ho wrote:
> On Wed, May 24, 2023 at 12:02 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > So the compiler warning/error needs to be fixed a different want.
> 
> Understood.  Would caping the length at iov_len with a ternary be sufficient?

The quoted text said 'string'. It is not clear if that means c-string,
with a trailing \0. If you just cap iov_len you could end up with a
string which is not terminated.

The other end of the socket should not blow up, because that would be
an obvious DOS or buffer overwrite attack vector. So you need to
decide, do you want to expose such issues and see if anything does
actually blow up, or do you want to do a bit more work and correctly
terminate the string when capped?

	Andrew

Re: [PATCH] Remove hardcoded static string length

[Kenny Ho]

On Wed, May 24, 2023 at 1:43 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> The other end of the socket should not blow up, because that would be
> an obvious DOS or buffer overwrite attack vector. So you need to
> decide, do you want to expose such issues and see if anything does
> actually blow up, or do you want to do a bit more work and correctly
> terminate the string when capped?

Right... I guess it's not clear to me that existing implementations
null-terminate correctly when UTS_RELEASE causes the string to exceed
the 65 byte size of rxrpc_version_string.  We can of course do better,
but I hesitate to do strncpy because I am not familiar with this code
base enough to tell if this function is part of some hot path where
strncpy matters.

Regards,
Kenny

RE: [PATCH] Remove hardcoded static string length

[David Laight]

From: Kenny Ho
> Sent: 24 May 2023 19:01
> 
> On Wed, May 24, 2023 at 1:43 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > The other end of the socket should not blow up, because that would be
> > an obvious DOS or buffer overwrite attack vector. So you need to
> > decide, do you want to expose such issues and see if anything does
> > actually blow up, or do you want to do a bit more work and correctly
> > terminate the string when capped?
> 
> Right... I guess it's not clear to me that existing implementations
> null-terminate correctly when UTS_RELEASE causes the string to exceed
> the 65 byte size of rxrpc_version_string.  We can of course do better,
> but I hesitate to do strncpy because I am not familiar with this code
> base enough to tell if this function is part of some hot path where
> strncpy matters.

The whole thing looks like it is expecting a max of 64 characters
and a terminating '\0'.
Since UTE_RELEASE goes in between two fixed strings truncating
the whole thing to 64/65 chars/bytes doesn't seem ideal.

I does rather beg the question as what is in UTS_RELEASE when
it exceeds (IIRC) about 48 characters?

If UTS_RELEASE is getting that long, it might easily exceed
the 64 characters returned by uname().

I suspect that you need to truncate UTS_RELEASE to limit
the string to 64 characters - so something like:
	static char id[65];
	if (!id[0])
		snprintf(id, sizeof id, "xxx-%.48s-yyy", UTS_RELEASE);

Using an on-stack buffer almost certainly wouldn't matter.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] Remove hardcoded static string length

[Kenny Ho]

On Thu, May 25, 2023 at 5:14 AM David Laight <David.Laight@aculab.com> wrote:
>
> I does rather beg the question as what is in UTS_RELEASE when
> it exceeds (IIRC) about 48 characters?

Thanks for the question as it made me dig deeper.  UTS_RELEASE is
actually capped at 64:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/Makefile?#n1317
"""
uts_len := 64
define filechk_utsrelease.h
if [ `echo -n "$(KERNELRELEASE)" | wc -c ` -gt $(uts_len) ]; then \
 echo '"$(KERNELRELEASE)" exceeds $(uts_len) characters' >&2;    \
 exit 1;                                                         \
...
"""

So UTS_RELEASE on its own would fit perfectly by coincidence (and it
is also why UTS_RELEASE with the pre and postfix exceeds the limit.)
That makes me wonder if the content / format of the version matter and
looks like it sort of does by looking at when the string was
introduced:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/net/rxrpc/local_object.c?id=44ba06987c0b10faa998b9324850e8a6564c714d

"The standard formulation seems to be: <project> <version> built
<yyyy>-<mm>-<dd>"

That commit also confirms the size and null termination requirement.

I will create a separate patch with your suggestion.

Regards,
Kenny

RE: [PATCH] Remove hardcoded static string length

[David Laight]

From: Kenny Ho
> Sent: 25 May 2023 15:28
> To: David Laight <David.Laight@ACULAB.COM>
> Cc: Andrew Lunn <andrew@lunn.ch>; Marc Dionne <marc.dionne@auristor.com>; Kenny Ho <Kenny.Ho@amd.com>;
> David Howells <dhowells@redhat.com>; David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; linux-
> afs@lists.infradead.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] Remove hardcoded static string length
> 
> On Thu, May 25, 2023 at 5:14 AM David Laight <David.Laight@aculab.com> wrote:
> >
> > I does rather beg the question as what is in UTS_RELEASE when
> > it exceeds (IIRC) about 48 characters?
> 
> Thanks for the question as it made me dig deeper.  UTS_RELEASE is
> actually capped at 64:
...

But isn't UTS_RELEASE usually much shorter?
I think it is what 'uname -r' prints, the longest I've seen recently
is "3.10.0-1127.19.1.el7.x86_64" - well under the limit.

...
> 
> "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__.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] Remove hardcoded static string length

[Kenny Ho]

On Thu, May 25, 2023 at 11:04 AM David Laight <David.Laight@aculab.com> wrote:
> But isn't UTS_RELEASE usually much shorter?
> I think it is what 'uname -r' prints, the longest I've seen recently
> is "3.10.0-1127.19.1.el7.x86_64" - well under the limit.

Usually yes, but I believe LOCALVERSION can be appended to
KERNELRELEASE / UTS_RELEASE which can makes it much longer.

> > "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

Re: [PATCH] Remove hardcoded static string length

[Jeffrey E Altman]

[-- Attachment #1: Type: text/plain, Size: 953 bytes --]

On 5/24/2023 1:43 PM, Andrew Lunn wrote:
> On Wed, May 24, 2023 at 01:02:36PM -0400, Kenny Ho wrote:
>> On Wed, May 24, 2023 at 12:02 PM Andrew Lunn <andrew@lunn.ch> wrote:
>>> So the compiler warning/error needs to be fixed a different want.
>> Understood.  Would caping the length at iov_len with a ternary be sufficient?
> The quoted text said 'string'. It is not clear if that means c-string,
> with a trailing \0. If you just cap iov_len you could end up with a
> string which is not terminated.
The expected buffer is a NUL terminated c-string.
> The other end of the socket should not blow up, because that would be
> an obvious DOS or buffer overwrite attack vector.

This is a valid concern because all versions of IBM AFS 3.6 Rx and 
OpenAFS Rx prior to 1.6.23 are susceptible to read beyond the end of 
buffer if either the received data is longer than 65 octets or the 
received data is 65 octets but not NUL terminated.

Jeffrey Altman



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4039 bytes --]

Re: [PATCH] Remove hardcoded static string length

[Jeffrey E Altman]

[-- Attachment #1.1: Type: text/plain, Size: 2019 bytes --]

On 5/25/2023 11:37 AM, Kenny Ho wrote:
> On Thu, May 25, 2023 at 11:04 AM David Laight<David.Laight@aculab.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.

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.
 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.

Jeffrey Altman


[-- Attachment #1.2: Type: text/html, Size: 2937 bytes --]

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4039 bytes --]

Re: [PATCH] Remove hardcoded static string length

[Jeffrey E Altman]

[-- Attachment #1: Type: text/plain, Size: 957 bytes --]

On 5/24/2023 1:43 PM, Andrew Lunn wrote:
> On Wed, May 24, 2023 at 01:02:36PM -0400, Kenny Ho wrote:
>> On Wed, May 24, 2023 at 12:02 PM Andrew Lunn <andrew@lunn.ch> wrote:
>>> So the compiler warning/error needs to be fixed a different want.
>> Understood. Would caping the length at iov_len with a ternary be 
>> sufficient?
> The quoted text said 'string'. It is not clear if that means c-string,
> with a trailing \0. If you just cap iov_len you could end up with a
> string which is not terminated.
The expected buffer is a NUL terminated c-string.
> The other end of the socket should not blow up, because that would be
> an obvious DOS or buffer overwrite attack vector.

This is a valid concern because all versions of IBM AFS 3.6 Rx and 
OpenAFS Rx prior to 1.6.23 are susceptible to read beyond the end of 
buffer if either the received data is longer than 65 octets or the 
received data is 65 octets but not NUL terminated.

Jeffrey Altman




[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4039 bytes --]

Re: [PATCH] Remove hardcoded static string length

[Jeffrey E Altman]

[-- Attachment #1: Type: text/plain, Size: 2020 bytes --]

On 5/25/2023 11:37 AM, Kenny Ho wrote:
> On Thu, May 25, 2023 at 11:04 AM David Laight<David.Laight@aculab.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.

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.
 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.

Jeffrey Altman



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4039 bytes --]

RE: [PATCH] Remove hardcoded static string length

[David Laight]

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@aculab.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?
Additionally is the response supposed to the '\0' terminated?
The existing code doesn't guarantee that at all.

> 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.

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....)

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.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] Remove hardcoded static string length

[Jeffrey E Altman]

[-- Attachment #1: Type: text/plain, Size: 6707 bytes --]

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@aculab.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



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4039 bytes --]