Remove hardcoded static string length

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

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.

Thanks for the link and the quote.

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

    Andrew

---
pw-bot: cr
  

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
  

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
  

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
  

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)
  

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
  

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)
  

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
  

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)