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

Review Request #447 — Created April 27, 2017 and submitted

marcel
illumos-gate
master
8106, 8109
905c868...
general
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 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.
  • 0
  • 0
  • 4
  • 0
  • 4
Description From Last Updated
vgusev
  1. Sorry, this patch has a lot of refactoring, etc.
    What is actual fixing change?

    I guess real fix is:

    1. Conditions "if (namelen > MAX_MACHINE_NAME) ..."
    2. 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.

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

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

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

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

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

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

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

  4. 
      
marcel
tsoome
  1. 
      
  2. usr/src/uts/common/rpc/sec/auth_kern.c (Diff revision 2)
     
     

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

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

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

  4. 
      
marcel
tsoome
  1. Ship It!
  2. 
      
jbk
  1. Ship It!
  2. 
      
marcel
Review request changed

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