As Bruce points out in RFC 4121, section 4.2.3:
"In Wrap tokens that provide for confidentiality, the first 16 octets
of the Wrap token (the "header", as defined in section 4.2.6), SHALL
be appended to the plaintext data before encryption. Filler octets
MAY be inserted between the plaintext data and the "header.""
...and...
"In Wrap tokens with confidentiality, the EC field SHALL be used to
encode the number of octets in the filler..."
It's possible for the client to stuff different data in that area on a
retransmission, which could make the checksum come out wrong in the DRC
code.
After decrypting the blob, we should trim off any extra count bytes in
addition to the checksum blob.
Reported-by: "J. Bruce Fields" <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
---
net/sunrpc/auth_gss/gss_krb5_wrap.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/sunrpc/auth_gss/gss_krb5_wrap.c b/net/sunrpc/auth_gss/gss_krb5_wrap.c
index 1da52d1..ec1f4d0 100644
--- a/net/sunrpc/auth_gss/gss_krb5_wrap.c
+++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c
@@ -574,8 +574,8 @@ gss_unwrap_kerberos_v2(struct krb5_ctx *kctx, int offset, struct xdr_buf *buf)
buf->head[0].iov_len -= GSS_KRB5_TOK_HDR_LEN + headskip;
buf->len -= GSS_KRB5_TOK_HDR_LEN + headskip;
- /* Trim off the checksum blob */
- xdr_buf_trim(buf, GSS_KRB5_TOK_HDR_LEN + tailskip);
+ /* Trim off the trailing "extra count" and checksum blob */
+ xdr_buf_trim(buf, ec + GSS_KRB5_TOK_HDR_LEN + tailskip);
return GSS_S_COMPLETE;
}
--
1.8.3.1
On Wed, 2013-10-16 at 14:53 -0400, Jeff Layton wrote:
+AD4- On Wed, 16 Oct 2013 18:43:26 +-0000
+AD4- +ACI-Myklebust, Trond+ACI- +ADw-Trond.Myklebust+AEA-netapp.com+AD4- wrote:
+AD4-
+AD4- +AD4- On Thu, 2013-10-10 at 13:56 -0400, Jeff Layton wrote:
+AD4- +AD4- +AD4- As Bruce points out in RFC 4121, section 4.2.3:
+AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- +ACI-In Wrap tokens that provide for confidentiality, the first 16 octets
+AD4- +AD4- +AD4- of the Wrap token (the +ACI-header+ACI-, as defined in section 4.2.6), SHALL
+AD4- +AD4- +AD4- be appended to the plaintext data before encryption. Filler octets
+AD4- +AD4- +AD4- MAY be inserted between the plaintext data and the +ACI-header.+ACIAIg-
+AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- ...and...
+AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- +ACI-In Wrap tokens with confidentiality, the EC field SHALL be used to
+AD4- +AD4- +AD4- encode the number of octets in the filler...+ACI-
+AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- It's possible for the client to stuff different data in that area on a
+AD4- +AD4- +AD4- retransmission, which could make the checksum come out wrong in the DRC
+AD4- +AD4- +AD4- code.
+AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- After decrypting the blob, we should trim off any extra count bytes in
+AD4- +AD4- +AD4- addition to the checksum blob.
+AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- Reported-by: +ACI-J. Bruce Fields+ACI- +ADw-bfields+AEA-fieldses.org+AD4-
+AD4- +AD4- +AD4- Signed-off-by: Jeff Layton +ADw-jlayton+AEA-redhat.com+AD4-
+AD4- +AD4- +AD4- ---
+AD4- +AD4- +AD4- net/sunrpc/auth+AF8-gss/gss+AF8-krb5+AF8-wrap.c +AHw- 4 +-+---
+AD4- +AD4- +AD4- 1 file changed, 2 insertions(+-), 2 deletions(-)
+AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- diff --git a/net/sunrpc/auth+AF8-gss/gss+AF8-krb5+AF8-wrap.c b/net/sunrpc/auth+AF8-gss/gss+AF8-krb5+AF8-wrap.c
+AD4- +AD4- +AD4- index 1da52d1..ec1f4d0 100644
+AD4- +AD4- +AD4- --- a/net/sunrpc/auth+AF8-gss/gss+AF8-krb5+AF8-wrap.c
+AD4- +AD4- +AD4- +-+-+- b/net/sunrpc/auth+AF8-gss/gss+AF8-krb5+AF8-wrap.c
+AD4- +AD4- +AD4- +AEAAQA- -574,8 +-574,8 +AEAAQA- gss+AF8-unwrap+AF8-kerberos+AF8-v2(struct krb5+AF8-ctx +ACo-kctx, int offset, struct xdr+AF8-buf +ACo-buf)
+AD4- +AD4- +AD4- buf-+AD4-head+AFs-0+AF0-.iov+AF8-len -+AD0- GSS+AF8-KRB5+AF8-TOK+AF8-HDR+AF8-LEN +- headskip+ADs-
+AD4- +AD4- +AD4- buf-+AD4-len -+AD0- GSS+AF8-KRB5+AF8-TOK+AF8-HDR+AF8-LEN +- headskip+ADs-
+AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- - /+ACo- Trim off the checksum blob +ACo-/
+AD4- +AD4- +AD4- - xdr+AF8-buf+AF8-trim(buf, GSS+AF8-KRB5+AF8-TOK+AF8-HDR+AF8-LEN +- tailskip)+ADs-
+AD4- +AD4- +AD4- +- /+ACo- Trim off the trailing +ACI-extra count+ACI- and checksum blob +ACo-/
+AD4- +AD4- +AD4- +- xdr+AF8-buf+AF8-trim(buf, ec +- GSS+AF8-KRB5+AF8-TOK+AF8-HDR+AF8-LEN +- tailskip)+ADs-
+AD4- +AD4- +AD4- return GSS+AF8-S+AF8-COMPLETE+ADs-
+AD4- +AD4- +AD4- +AH0-
+AD4- +AD4- +AD4-
+AD4- +AD4-
+AD4- +AD4- If this is just padding, then why would it be anything other than '0',
+AD4- +AD4- even on a retransmission? Clients are not supposed to leak random data
+AD4- +AD4- to the server...
+AD4- +AD4-
+AD4-
+AD4- The Linux client routines are actually set up to fill this with 'X'
+AD4- characters, not nulls. See gss+AF8-krb5+AF8-aes+AF8-encrypt, which does this:
+AD4-
+AD4- memset(ecptr, 'X', ec)+ADs-
+AD4-
+AD4- AFAICT, the spec doesn't actually say what's supposed to be in the
+AD4- padding, just that it's to be ignored. I imagine that most clients will
+AD4- just set this to a constant value like Linux does, but there doesn't
+AD4- seem to be any guarantee of that, and there's no real point in keeping
+AD4- it around.
The only caller of gss+AF8-krb5+AF8-aes+AF8-encrypt is gss+AF8-wrap+AF8-kerberos+AF8-v2(), which
sets 'ec' to 0.
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust+AEA-netapp.com
http://www.netapp.com
On Wed, 16 Oct 2013 18:43:26 +0000
"Myklebust, Trond" <[email protected]> wrote:
> On Thu, 2013-10-10 at 13:56 -0400, Jeff Layton wrote:
> > As Bruce points out in RFC 4121, section 4.2.3:
> >
> > "In Wrap tokens that provide for confidentiality, the first 16 octets
> > of the Wrap token (the "header", as defined in section 4.2.6), SHALL
> > be appended to the plaintext data before encryption. Filler octets
> > MAY be inserted between the plaintext data and the "header.""
> >
> > ...and...
> >
> > "In Wrap tokens with confidentiality, the EC field SHALL be used to
> > encode the number of octets in the filler..."
> >
> > It's possible for the client to stuff different data in that area on a
> > retransmission, which could make the checksum come out wrong in the DRC
> > code.
> >
> > After decrypting the blob, we should trim off any extra count bytes in
> > addition to the checksum blob.
> >
> > Reported-by: "J. Bruce Fields" <[email protected]>
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > net/sunrpc/auth_gss/gss_krb5_wrap.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/sunrpc/auth_gss/gss_krb5_wrap.c b/net/sunrpc/auth_gss/gss_krb5_wrap.c
> > index 1da52d1..ec1f4d0 100644
> > --- a/net/sunrpc/auth_gss/gss_krb5_wrap.c
> > +++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c
> > @@ -574,8 +574,8 @@ gss_unwrap_kerberos_v2(struct krb5_ctx *kctx, int offset, struct xdr_buf *buf)
> > buf->head[0].iov_len -= GSS_KRB5_TOK_HDR_LEN + headskip;
> > buf->len -= GSS_KRB5_TOK_HDR_LEN + headskip;
> >
> > - /* Trim off the checksum blob */
> > - xdr_buf_trim(buf, GSS_KRB5_TOK_HDR_LEN + tailskip);
> > + /* Trim off the trailing "extra count" and checksum blob */
> > + xdr_buf_trim(buf, ec + GSS_KRB5_TOK_HDR_LEN + tailskip);
> > return GSS_S_COMPLETE;
> > }
> >
>
> If this is just padding, then why would it be anything other than '0',
> even on a retransmission? Clients are not supposed to leak random data
> to the server...
>
The Linux client routines are actually set up to fill this with 'X'
characters, not nulls. See gss_krb5_aes_encrypt, which does this:
memset(ecptr, 'X', ec);
AFAICT, the spec doesn't actually say what's supposed to be in the
padding, just that it's to be ignored. I imagine that most clients will
just set this to a constant value like Linux does, but there doesn't
seem to be any guarantee of that, and there's no real point in keeping
it around.
--
Jeff Layton <[email protected]>
On Thu, Oct 10, 2013 at 01:56:50PM -0400, Jeff Layton wrote:
> As Bruce points out in RFC 4121, section 4.2.3:
>
> "In Wrap tokens that provide for confidentiality, the first 16 octets
> of the Wrap token (the "header", as defined in section 4.2.6), SHALL
> be appended to the plaintext data before encryption. Filler octets
> MAY be inserted between the plaintext data and the "header.""
>
> ...and...
>
> "In Wrap tokens with confidentiality, the EC field SHALL be used to
> encode the number of octets in the filler..."
>
> It's possible for the client to stuff different data in that area on a
> retransmission, which could make the checksum come out wrong in the DRC
> code.
>
> After decrypting the blob, we should trim off any extra count bytes in
> addition to the checksum blob.
>
> Reported-by: "J. Bruce Fields" <[email protected]>
> Signed-off-by: Jeff Layton <[email protected]>
Thanks, applying for 3.13.--b.
> ---
> net/sunrpc/auth_gss/gss_krb5_wrap.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/sunrpc/auth_gss/gss_krb5_wrap.c b/net/sunrpc/auth_gss/gss_krb5_wrap.c
> index 1da52d1..ec1f4d0 100644
> --- a/net/sunrpc/auth_gss/gss_krb5_wrap.c
> +++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c
> @@ -574,8 +574,8 @@ gss_unwrap_kerberos_v2(struct krb5_ctx *kctx, int offset, struct xdr_buf *buf)
> buf->head[0].iov_len -= GSS_KRB5_TOK_HDR_LEN + headskip;
> buf->len -= GSS_KRB5_TOK_HDR_LEN + headskip;
>
> - /* Trim off the checksum blob */
> - xdr_buf_trim(buf, GSS_KRB5_TOK_HDR_LEN + tailskip);
> + /* Trim off the trailing "extra count" and checksum blob */
> + xdr_buf_trim(buf, ec + GSS_KRB5_TOK_HDR_LEN + tailskip);
> return GSS_S_COMPLETE;
> }
>
> --
> 1.8.3.1
>
On Wed, Oct 16, 2013 at 06:43:26PM +0000, Myklebust, Trond wrote:
> On Thu, 2013-10-10 at 13:56 -0400, Jeff Layton wrote:
> > As Bruce points out in RFC 4121, section 4.2.3:
> >
> > "In Wrap tokens that provide for confidentiality, the first 16 octets
> > of the Wrap token (the "header", as defined in section 4.2.6), SHALL
> > be appended to the plaintext data before encryption. Filler octets
> > MAY be inserted between the plaintext data and the "header.""
> >
> > ...and...
> >
> > "In Wrap tokens with confidentiality, the EC field SHALL be used to
> > encode the number of octets in the filler..."
> >
> > It's possible for the client to stuff different data in that area on a
> > retransmission, which could make the checksum come out wrong in the DRC
> > code.
> >
> > After decrypting the blob, we should trim off any extra count bytes in
> > addition to the checksum blob.
> >
> > Reported-by: "J. Bruce Fields" <[email protected]>
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > net/sunrpc/auth_gss/gss_krb5_wrap.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/sunrpc/auth_gss/gss_krb5_wrap.c b/net/sunrpc/auth_gss/gss_krb5_wrap.c
> > index 1da52d1..ec1f4d0 100644
> > --- a/net/sunrpc/auth_gss/gss_krb5_wrap.c
> > +++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c
> > @@ -574,8 +574,8 @@ gss_unwrap_kerberos_v2(struct krb5_ctx *kctx, int offset, struct xdr_buf *buf)
> > buf->head[0].iov_len -= GSS_KRB5_TOK_HDR_LEN + headskip;
> > buf->len -= GSS_KRB5_TOK_HDR_LEN + headskip;
> >
> > - /* Trim off the checksum blob */
> > - xdr_buf_trim(buf, GSS_KRB5_TOK_HDR_LEN + tailskip);
> > + /* Trim off the trailing "extra count" and checksum blob */
> > + xdr_buf_trim(buf, ec + GSS_KRB5_TOK_HDR_LEN + tailskip);
> > return GSS_S_COMPLETE;
> > }
> >
>
> If this is just padding, then why would it be anything other than '0',
> even on a retransmission? Clients are not supposed to leak random data
> to the server...
Right, the problem wasn't random padding data.
The problem was just with the xdr_buf length being wrong. The nfsd drc
uses the length, and will notice if the length differs between two
otherwise identical rpc's. So you'd have to have a pretty weird client
gss implementation, one that not only uses EC but that for some reason
uses a different EC value on retransmission. I don't know if that's
even possible.
Currently there doesn't seem to be any other code that cares if buf->len
is right(the xdr decoding would only notice if buf->len is too short,
not if it's too long), but we should still try to get it right....
--b.
On Thu, 2013-10-10 at 13:56 -0400, Jeff Layton wrote:
+AD4- As Bruce points out in RFC 4121, section 4.2.3:
+AD4-
+AD4- +ACI-In Wrap tokens that provide for confidentiality, the first 16 octets
+AD4- of the Wrap token (the +ACI-header+ACI-, as defined in section 4.2.6), SHALL
+AD4- be appended to the plaintext data before encryption. Filler octets
+AD4- MAY be inserted between the plaintext data and the +ACI-header.+ACIAIg-
+AD4-
+AD4- ...and...
+AD4-
+AD4- +ACI-In Wrap tokens with confidentiality, the EC field SHALL be used to
+AD4- encode the number of octets in the filler...+ACI-
+AD4-
+AD4- It's possible for the client to stuff different data in that area on a
+AD4- retransmission, which could make the checksum come out wrong in the DRC
+AD4- code.
+AD4-
+AD4- After decrypting the blob, we should trim off any extra count bytes in
+AD4- addition to the checksum blob.
+AD4-
+AD4- Reported-by: +ACI-J. Bruce Fields+ACI- +ADw-bfields+AEA-fieldses.org+AD4-
+AD4- Signed-off-by: Jeff Layton +ADw-jlayton+AEA-redhat.com+AD4-
+AD4- ---
+AD4- net/sunrpc/auth+AF8-gss/gss+AF8-krb5+AF8-wrap.c +AHw- 4 +-+---
+AD4- 1 file changed, 2 insertions(+-), 2 deletions(-)
+AD4-
+AD4- diff --git a/net/sunrpc/auth+AF8-gss/gss+AF8-krb5+AF8-wrap.c b/net/sunrpc/auth+AF8-gss/gss+AF8-krb5+AF8-wrap.c
+AD4- index 1da52d1..ec1f4d0 100644
+AD4- --- a/net/sunrpc/auth+AF8-gss/gss+AF8-krb5+AF8-wrap.c
+AD4- +-+-+- b/net/sunrpc/auth+AF8-gss/gss+AF8-krb5+AF8-wrap.c
+AD4- +AEAAQA- -574,8 +-574,8 +AEAAQA- gss+AF8-unwrap+AF8-kerberos+AF8-v2(struct krb5+AF8-ctx +ACo-kctx, int offset, struct xdr+AF8-buf +ACo-buf)
+AD4- buf-+AD4-head+AFs-0+AF0-.iov+AF8-len -+AD0- GSS+AF8-KRB5+AF8-TOK+AF8-HDR+AF8-LEN +- headskip+ADs-
+AD4- buf-+AD4-len -+AD0- GSS+AF8-KRB5+AF8-TOK+AF8-HDR+AF8-LEN +- headskip+ADs-
+AD4-
+AD4- - /+ACo- Trim off the checksum blob +ACo-/
+AD4- - xdr+AF8-buf+AF8-trim(buf, GSS+AF8-KRB5+AF8-TOK+AF8-HDR+AF8-LEN +- tailskip)+ADs-
+AD4- +- /+ACo- Trim off the trailing +ACI-extra count+ACI- and checksum blob +ACo-/
+AD4- +- xdr+AF8-buf+AF8-trim(buf, ec +- GSS+AF8-KRB5+AF8-TOK+AF8-HDR+AF8-LEN +- tailskip)+ADs-
+AD4- return GSS+AF8-S+AF8-COMPLETE+ADs-
+AD4- +AH0-
+AD4-
If this is just padding, then why would it be anything other than '0',
even on a retransmission? Clients are not supposed to leak random data
to the server...
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust+AEA-netapp.com
http://www.netapp.com
On Wed, 16 Oct 2013 19:09:48 +0000
"Myklebust, Trond" <[email protected]> wrote:
> On Wed, 2013-10-16 at 14:53 -0400, Jeff Layton wrote:
> > On Wed, 16 Oct 2013 18:43:26 +0000
> > "Myklebust, Trond" <[email protected]> wrote:
> >
> > > On Thu, 2013-10-10 at 13:56 -0400, Jeff Layton wrote:
> > > > As Bruce points out in RFC 4121, section 4.2.3:
> > > >
> > > > "In Wrap tokens that provide for confidentiality, the first 16 octets
> > > > of the Wrap token (the "header", as defined in section 4.2.6), SHALL
> > > > be appended to the plaintext data before encryption. Filler octets
> > > > MAY be inserted between the plaintext data and the "header.""
> > > >
> > > > ...and...
> > > >
> > > > "In Wrap tokens with confidentiality, the EC field SHALL be used to
> > > > encode the number of octets in the filler..."
> > > >
> > > > It's possible for the client to stuff different data in that area on a
> > > > retransmission, which could make the checksum come out wrong in the DRC
> > > > code.
> > > >
> > > > After decrypting the blob, we should trim off any extra count bytes in
> > > > addition to the checksum blob.
> > > >
> > > > Reported-by: "J. Bruce Fields" <[email protected]>
> > > > Signed-off-by: Jeff Layton <[email protected]>
> > > > ---
> > > > net/sunrpc/auth_gss/gss_krb5_wrap.c | 4 ++--
> > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/net/sunrpc/auth_gss/gss_krb5_wrap.c b/net/sunrpc/auth_gss/gss_krb5_wrap.c
> > > > index 1da52d1..ec1f4d0 100644
> > > > --- a/net/sunrpc/auth_gss/gss_krb5_wrap.c
> > > > +++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c
> > > > @@ -574,8 +574,8 @@ gss_unwrap_kerberos_v2(struct krb5_ctx *kctx, int offset, struct xdr_buf *buf)
> > > > buf->head[0].iov_len -= GSS_KRB5_TOK_HDR_LEN + headskip;
> > > > buf->len -= GSS_KRB5_TOK_HDR_LEN + headskip;
> > > >
> > > > - /* Trim off the checksum blob */
> > > > - xdr_buf_trim(buf, GSS_KRB5_TOK_HDR_LEN + tailskip);
> > > > + /* Trim off the trailing "extra count" and checksum blob */
> > > > + xdr_buf_trim(buf, ec + GSS_KRB5_TOK_HDR_LEN + tailskip);
> > > > return GSS_S_COMPLETE;
> > > > }
> > > >
> > >
> > > If this is just padding, then why would it be anything other than '0',
> > > even on a retransmission? Clients are not supposed to leak random data
> > > to the server...
> > >
> >
> > The Linux client routines are actually set up to fill this with 'X'
> > characters, not nulls. See gss_krb5_aes_encrypt, which does this:
> >
> > memset(ecptr, 'X', ec);
> >
> > AFAICT, the spec doesn't actually say what's supposed to be in the
> > padding, just that it's to be ignored. I imagine that most clients will
> > just set this to a constant value like Linux does, but there doesn't
> > seem to be any guarantee of that, and there's no real point in keeping
> > it around.
>
> The only caller of gss_krb5_aes_encrypt is gss_wrap_kerberos_v2(), which
> sets 'ec' to 0.
>
Yes, I realize that. The Linux client currently never sends any
padding. I was just pointing out that the encrypt_v2 routine is set up
to handle a non-zero "ec" value, and that when it gets one it fills the
padding with X's. We could make that not even pass in an "ec" value at
all of course, but maybe it'll come in handy sometime.
--
Jeff Layton <[email protected]>