- Download Diff

Summary:

AUTH_SYS and AUTH_LOOPBACK marshalling fixes in the kernel RPC client code (8106, 8109)

Review Request #447 — Created April 27, 2017 and submitted June 13, 2017, 4:31 p.m.

Information
Owner:	marcel
Repository:	illumos-gate
Branch:	master
Bugs:	8106, 8109
Depends On:
Commit:	905c868...
Reviewers
Groups:	general
People:

Description

This fixes two bugs in the kernel AUTH_SYS and AUTH_LOOPBACK marshalling code.
Only the RPC client side is affected.

Bug #8106 addresses a case when the authloopback_marshal() function can
generate too big authentication data in the output stream.  Such stream
violates RFC 5531 and should not by accepted by any conforming RPC server.
With the fix the oversized auth data are not generated and the
authloopback_marshal() function fails.

In #8109 the kernel AUTH_SYS and AUTH_LOOPBACK implementation can ignore
provided credentials in some special cases and use credentials of the current
thread instead.  This can cause the RPC operation failure due to authentication
issues for kernel RPC clients.  Fortunately, I didn't found any code path that
might trigger this issue, but I must admit that I didn't tried very hard.

Testing Done

Testing using the module.c file attached to bug #8106:

Without the fix:

Apr 26 16:00:37 t4 module: [ID 378104 kern.info] NOTICE: AUTH_LOOPBACK, 70 groups, xdrmem success: 000055de 00000190
Apr 26 16:00:37 t4 module: [ID 618828 kern.info] NOTICE: AUTH_LOOPBACK, 70 groups, xdrmblk success: 000055de 000000a4
Apr 26 16:00:37 t4 module: [ID 633239 kern.info] NOTICE: AUTH_LOOPBACK, 80 groups, xdrmem success: 000055de 000001b8
Apr 26 16:00:37 t4 module: [ID 767423 kern.info] NOTICE: AUTH_LOOPBACK, 80 groups, xdrmblk success: 000055de 000000a4
Apr 26 16:00:37 t4 module: [ID 398392 kern.info] NOTICE: AUTH_SYS, 80 groups, xdrmem success: 00000001 000000b8
Apr 26 16:00:37 t4 module: [ID 301671 kern.info] NOTICE: AUTH_SYS, 80 groups, xdrmblk success: 00000001 000000a4

With the fix:

Apr 27 16:12:54 openindiana module: [ID 378104 kern.info] NOTICE: AUTH_LOOPBACK, 70 groups, xdrmem success: 000055de 00000190
Apr 27 16:12:54 openindiana module: [ID 618828 kern.info] NOTICE: AUTH_LOOPBACK, 70 groups, xdrmblk success: 000055de 00000190
Apr 27 16:12:54 openindiana module: [ID 983455 kern.info] NOTICE: AUTH_LOOPBACK, 80 groups, xdrmem FAILED
Apr 27 16:12:54 openindiana module: [ID 575630 kern.info] NOTICE: AUTH_LOOPBACK, 80 groups, xdrmblk FAILED
Apr 27 16:12:54 openindiana module: [ID 398392 kern.info] NOTICE: AUTH_SYS, 80 groups, xdrmem success: 00000001 000000b8
Apr 27 16:12:54 openindiana module: [ID 301671 kern.info] NOTICE: AUTH_SYS, 80 groups, xdrmblk success: 00000001 000000b8

I also tested the basic NFS functionality to make sure there is no regression introduced by this fix.

Issues

0
0
4
0
All Issues: 4

Description	From	Last Updated
Note to myself: If I'll update this change I should add an assert here that we are encoding. Something like ...	marcel	May 11, 2017, 2:21 p.m.
Note to myself: If I'll update this change I should add an assert here that we are encoding. Something like ...	marcel	May 11, 2017, 2:21 p.m.
perhaps it is better to #define the name for the constant? NFIELDS or something...	tsoome	May 24, 2017, 5:35 a.m.
it will help the reader to use (rounded_namelen - namelen) > 0 - of course it is just minor nit:)	tsoome	May 24, 2017, 5:35 a.m.

Sorry, this patch has a lot of refactoring, etc.

What is actual fixing change?
I guess real fix is:

Conditions "if (namelen > MAX_MACHINE_NAME) ..."
Handling @cr in xdr_authkern() and xdr_authloopback().


Additionally want to note, authkern_marshal() and authloopback_marshal() has almost identical code that was copypasted and it would be nice to have base code that calls passed fn:  xdr_authkern or  xdr_authloopback.

marcel May 11, 2017, 9:39 a.m.

There are some major fixes and many minor fixes here.  The major ones are those two you listed and the change from gidlen > NGRPS_LOOPBACK to gidlen > maxgidlen in authloopback_marshal().  Everything else are just fixes for minor issues (IIRC).
Yes, both authkern_marshal() and authloopback_marshal() are almost the same, but I'd prefer to leave them separate.  At least for now.

usr/src/uts/common/rpc/sec/auth_kern.c (Diff revision 1)

It looks like this is excessive. startpos is always zero here. Isn't it?

marcel May 11, 2017, 9:39 a.m.

Yes, with the current implementation of xdrmem the startpos value is always zero here, but nothing quarantees that, so it is better to be safe.

usr/src/uts/common/rpc/sec/auth_kern.c (Diff revision 1)

Show all issues

Note to myself: If I'll update this change I should add an assert here that we are encoding.  Something like ASSERT(xdrs->x_op == XDR_ENCODE).

usr/src/uts/common/rpc/sec/auth_loopb.c (Diff revision 1)

Show all issues

Note to myself: If I'll update this change I should add an assert here that we are encoding.  Something like ASSERT(xdrs->x_op == XDR_ENCODE).

Change Summary:

Added ASSERT(xdrs->x_op == XDR_ENCODE).

Commit:

-	723e4faf04f1c079ba6410c7dd598d1b6ab6f675
+	a96b53ee9065623c69280f05ab4ff05bbaaa2e0a

Diff:

Revision 2 (+155 -82)

Show changes

	usr/src/uts/common/rpc/auth_sys.h
	usr/src/uts/common/rpc/sec/auth_kern.c
	usr/src/uts/common/rpc/sec/auth_loopb.c
	usr/src/uts/common/rpc/sec/authu_prot.c

usr/src/uts/common/rpc/sec/auth_kern.c (Diff revision 2)

Show all issues

perhaps it is better to #define the name for the constant? NFIELDS or something...

usr/src/uts/common/rpc/sec/auth_kern.c (Diff revision 2)

Show all issues

it will help the reader to use (rounded_namelen - namelen) > 0 - of course it is just minor nit:)

Change Summary:

Feedback from Toomas.

Description:

		This fixes two bugs in the kernel AUTH_SYS and AUTH_LOOPBACK marshalling code.
		Only the RPC client side is affected.

~		Bug #8106 addresses a case when the authloopback_marshal() function can
	~	Bug #8106 addresses a case when the authloopback_marshal() function can
		generate too big authentication data in the output stream. Such stream
		violates RFC 5531 and should not by accepted by any conforming RPC server.
		With the fix the oversized auth data are not generated and the
		authloopback_marshal() function fails.

		In #8109 the kernel AUTH_SYS and AUTH_LOOPBACK implementation can ignore
		provided credentials in some special cases and use credentials of the current
		thread instead. This can cause the RPC operation failure due to authentication
		issues for kernel RPC clients. Fortunately, I didn't found any code path that
		might trigger this issue, but I must admit that I didn't tried very hard.

Commit:

-	a96b53ee9065623c69280f05ab4ff05bbaaa2e0a
+	905c8688997e37abc9d31eb51204de564fd8b49e

Diff:

Revision 3 (+157 -82)

Show changes

	usr/src/uts/common/rpc/auth_sys.h
	usr/src/uts/common/rpc/sec/auth_kern.c
	usr/src/uts/common/rpc/sec/auth_loopb.c
	usr/src/uts/common/rpc/sec/authu_prot.c

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

commit 6dd72a43d2e43185833c20e7f0c4cb88a4d37ec8
Author:     Marcel Telka <marcel@telka.sk>
AuthorDate: Wed May 24 07:32:14 2017 +0200
Commit:     Dan McDonald <danmcd@joyent.com>
CommitDate: Tue Jun 13 10:37:46 2017 -0400

    8106 authloopback_marshal() can violate the RPC specification
    8109 Kernel AUTH_SYS and AUTH_LOOPBACK implementation can ignore provided credentials
    Reviewed by: Toomas Soome <tsoome@me.com>
    Reviewed by: Jason King <jason.brian.king+illumos@gmail.com>
    Approved by: Dan McDonald <danmcd@joyent.com>

:100644 100644 5f163d2... ef66402... M	usr/src/uts/common/rpc/auth_sys.h
:100644 100644 e045c1c... 2c3286d... M	usr/src/uts/common/rpc/sec/auth_kern.c
:100644 100644 8e4e452... ab53181... M	usr/src/uts/common/rpc/sec/auth_loopb.c
:100644 100644 dbc719e... 8b0bf90... M	usr/src/uts/common/rpc/sec/authu_prot.c

You have a pending review.

Review Board 3.0.18

AUTH_SYS and AUTH_LOOPBACK marshalling fixes in the kernel RPC client code (8106, 8109)

Change Summary:

Commit:

Diff:

Change Summary:

Description:

Commit:

Diff:

Status: Closed (submitted)

Change Summary: