linux-toradex.git/security/commoncap.c, branch v4.16-rc4

capabilities: fix buffer overread on very short xattr

2018-01-02T09:49:13+00:00

If userspace attempted to set a "security.capability" xattr shorter than
4 bytes (e.g. 'setfattr -n security.capability -v x file'), then
cap_convert_nscap() read past the end of the buffer containing the xattr
value because it accessed the ->magic_etc field without verifying that
the xattr value is long enough to contain that field.

Fix it by validating the xattr value size first.

This bug was found using syzkaller with KASAN.  The KASAN report was as
follows (cleaned up slightly):

    BUG: KASAN: slab-out-of-bounds in cap_convert_nscap+0x514/0x630 security/commoncap.c:498
    Read of size 4 at addr ffff88002d8741c0 by task syz-executor1/2852

    CPU: 0 PID: 2852 Comm: syz-executor1 Not tainted 4.15.0-rc6-00200-gcc0aac99d977 #253
    Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.11.0-20171110_100015-anatol 04/01/2014
    Call Trace:
     __dump_stack lib/dump_stack.c:17 [inline]
     dump_stack+0xe3/0x195 lib/dump_stack.c:53
     print_address_description+0x73/0x260 mm/kasan/report.c:252
     kasan_report_error mm/kasan/report.c:351 [inline]
     kasan_report+0x235/0x350 mm/kasan/report.c:409
     cap_convert_nscap+0x514/0x630 security/commoncap.c:498
     setxattr+0x2bd/0x350 fs/xattr.c:446
     path_setxattr+0x168/0x1b0 fs/xattr.c:472
     SYSC_setxattr fs/xattr.c:487 [inline]
     SyS_setxattr+0x36/0x50 fs/xattr.c:483
     entry_SYSCALL_64_fastpath+0x18/0x85

Fixes: 8db6c34f1dbc ("Introduce v3 namespaced file capabilities")
Cc:  # v4.14+
Signed-off-by: Eric Biggers 
Reviewed-by: Serge Hallyn 
Signed-off-by: James Morris

Merge branch 'next-general' of git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security

2017-11-13T18:30:44+00:00

Pull general security subsystem updates from James Morris:
 "TPM (from Jarkko):
   - essential clean up for tpm_crb so that ARM64 and x86 versions do
     not distract each other as much as before

   - /dev/tpm0 rejects now too short writes (shorter buffer than
     specified in the command header

   - use DMA-safe buffer in tpm_tis_spi

   - otherwise mostly minor fixes.

  Smack:
   - base support for overlafs

  Capabilities:
   - BPRM_FCAPS fixes, from Richard Guy Briggs:

     The audit subsystem is adding a BPRM_FCAPS record when auditing
     setuid application execution (SYSCALL execve). This is not expected
     as it was supposed to be limited to when the file system actually
     had capabilities in an extended attribute. It lists all
     capabilities making the event really ugly to parse what is
     happening. The PATH record correctly records the setuid bit and
     owner. Suppress the BPRM_FCAPS record on set*id.

  TOMOYO:
   - Y2038 timestamping fixes"

* 'next-general' of git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security: (28 commits)
  MAINTAINERS: update the IMA, EVM, trusted-keys, encrypted-keys entries
  Smack: Base support for overlayfs
  MAINTAINERS: remove David Safford as maintainer for encrypted+trusted keys
  tomoyo: fix timestamping for y2038
  capabilities: audit log other surprising conditions
  capabilities: fix logic for effective root or real root
  capabilities: invert logic for clarity
  capabilities: remove a layer of conditional logic
  capabilities: move audit log decision to function
  capabilities: use intuitive names for id changes
  capabilities: use root_priveleged inline to clarify logic
  capabilities: rename has_cap to has_fcap
  capabilities: intuitive names for cap gain status
  capabilities: factor out cap_bprm_set_creds privileged root
  tpm, tpm_tis: use ARRAY_SIZE() to define TPM_HID_USR_IDX
  tpm: fix duplicate inline declaration specifier
  tpm: fix type of a local variables in tpm_tis_spi.c
  tpm: fix type of a local variable in tpm2_map_command()
  tpm: fix type of a local variable in tpm2_get_cc_attrs_tbl()
  tpm-dev-common: Reject too short writes
  ...

capabilities: audit log other surprising conditions

2017-10-20T04:22:46+00:00

The existing condition tested for process effective capabilities set by
file attributes but intended to ignore the change if the result was
unsurprisingly an effective full set in the case root is special with a
setuid root executable file and we are root.

Stated again:
- When you execute a setuid root application, it is no surprise and
  expected that it got all capabilities, so we do not want capabilities
  recorded.
        if (pE_grew && !(pE_fullset && (eff_root || real_root) && root_priveleged) )

Now make sure we cover other cases:
- If something prevented a setuid root app getting all capabilities and
  it wound up with one capability only, then it is a surprise and should
  be logged.  When it is a setuid root file, we only want capabilities
  when the process does not get full capabilities..
        root_priveleged && setuid_root && !pE_fullset

- Similarly if a non-setuid program does pick up capabilities due to
  file system based capabilities, then we want to know what capabilities
  were picked up.  When it has file system based capabilities we want
  the capabilities.
        !is_setuid && (has_fcap && pP_gained)

- If it is a non-setuid file and it gets ambient capabilities, we want
  the capabilities.
        !is_setuid && pA_gained

- These last two are combined into one due to the common first parameter.

Related: https://github.com/linux-audit/audit-kernel/issues/16

Signed-off-by: Richard Guy Briggs 
Reviewed-by: Serge Hallyn 
Acked-by: James Morris 
Acked-by: Kees Cook 
Acked-by: Paul Moore 
Signed-off-by: James Morris

capabilities: fix logic for effective root or real root

2017-10-20T04:22:45+00:00

Now that the logic is inverted, it is much easier to see that both real
root and effective root conditions had to be met to avoid printing the
BPRM_FCAPS record with audit syscalls.  This meant that any setuid root
applications would print a full BPRM_FCAPS record when it wasn't
necessary, cluttering the event output, since the SYSCALL and PATH
records indicated the presence of the setuid bit and effective root user
id.

Require only one of effective root or real root to avoid printing the
unnecessary record.

Ref: commit 3fc689e96c0c ("Add audit_log_bprm_fcaps/AUDIT_BPRM_FCAPS")
See: https://github.com/linux-audit/audit-kernel/issues/16

Signed-off-by: Richard Guy Briggs 
Reviewed-by: Serge Hallyn 
Acked-by: James Morris 
Acked-by: Kees Cook 
Acked-by: Paul Moore 
Signed-off-by: James Morris

capabilities: invert logic for clarity

2017-10-20T04:22:45+00:00

The way the logic was presented, it was awkward to read and verify.
Invert the logic using DeMorgan's Law to be more easily able to read and
understand.

Signed-off-by: Richard Guy Briggs 
Reviewed-by: Serge Hallyn 
Acked-by: James Morris 
Acked-by: Kees Cook 
Okay-ished-by: Paul Moore 
Signed-off-by: James Morris

capabilities: remove a layer of conditional logic

2017-10-20T04:22:45+00:00

Remove a layer of conditional logic to make the use of conditions
easier to read and analyse.

Signed-off-by: Richard Guy Briggs 
Reviewed-by: Serge Hallyn 
Acked-by: James Morris 
Acked-by: Kees Cook 
Okay-ished-by: Paul Moore 
Signed-off-by: James Morris

capabilities: move audit log decision to function

2017-10-20T04:22:44+00:00

Move the audit log decision logic to its own function to isolate the
complexity in one place.

Suggested-by: Serge Hallyn 
Signed-off-by: Richard Guy Briggs 
Reviewed-by: Serge Hallyn 
Acked-by: James Morris 
Acked-by: Kees Cook 
Okay-ished-by: Paul Moore 
Signed-off-by: James Morris

capabilities: use intuitive names for id changes

2017-10-20T04:22:44+00:00

Introduce a number of inlines to make the use of the negation of
uid_eq() easier to read and analyse.

Signed-off-by: Richard Guy Briggs 
Reviewed-by: Serge Hallyn 
Acked-by: James Morris 
Acked-by: Kees Cook 
Okay-ished-by: Paul Moore 
Signed-off-by: James Morris

capabilities: use root_priveleged inline to clarify logic

2017-10-20T04:22:44+00:00

Introduce inline root_privileged() to make use of SECURE_NONROOT
easier to read.

Suggested-by: Serge Hallyn 
Signed-off-by: Richard Guy Briggs 
Reviewed-by: Serge Hallyn 
Acked-by: James Morris 
Acked-by: Kees Cook 
Okay-ished-by: Paul Moore 
Signed-off-by: James Morris

capabilities: rename has_cap to has_fcap

2017-10-20T04:22:44+00:00

Rename has_cap to has_fcap to clarify it applies to file capabilities
since the entire source file is about capabilities.

Signed-off-by: Richard Guy Briggs 
Reviewed-by: Serge Hallyn 
Acked-by: James Morris 
Acked-by: Kees Cook 
Okay-ished-by: Paul Moore 
Signed-off-by: James Morris