diff options
author | rwatson <rwatson@FreeBSD.org> | 2006-07-03 14:55:55 +0000 |
---|---|---|
committer | rwatson <rwatson@FreeBSD.org> | 2006-07-03 14:55:55 +0000 |
commit | 981c1cc4c81e29d3d6d4d5e2969b5cb584e427e9 (patch) | |
tree | 9b67eb5d06afb3e92bdc082c7ba9b83d69f20fd3 /sys/security/audit | |
parent | 0ba9449007ebda4eecb8886849779ef0010d0823 (diff) | |
download | FreeBSD-src-981c1cc4c81e29d3d6d4d5e2969b5cb584e427e9.zip FreeBSD-src-981c1cc4c81e29d3d6d4d5e2969b5cb584e427e9.tar.gz |
Correct a number of problems that were previously commented on:
- Correct audit_arg_socketaddr() argument name from so to sa.
- Assert arguments are non-NULL to many argument capture functions
rather than testing them. This may trip some bugs.
- Assert the process lock is held when auditing process
information.
- Test currecord in several more places.
- Test validity of more arguments with kasserts, such as flag
values when auditing vnode information.
Perforce change: 98825
Obtained from: TrustedBSD Project
Diffstat (limited to 'sys/security/audit')
-rw-r--r-- | sys/security/audit/audit.h | 2 | ||||
-rw-r--r-- | sys/security/audit/audit_arg.c | 87 |
2 files changed, 40 insertions, 49 deletions
diff --git a/sys/security/audit/audit.h b/sys/security/audit/audit.h index df74e9a..6043236 100644 --- a/sys/security/audit/audit.h +++ b/sys/security/audit/audit.h @@ -151,7 +151,7 @@ void audit_arg_pid(pid_t pid); void audit_arg_process(struct proc *p); void audit_arg_signum(u_int signum); void audit_arg_socket(int sodomain, int sotype, int soprotocol); -void audit_arg_sockaddr(struct thread *td, struct sockaddr *so); +void audit_arg_sockaddr(struct thread *td, struct sockaddr *sa); void audit_arg_auid(uid_t auid); void audit_arg_auditinfo(struct auditinfo *au_info); void audit_arg_upath(struct thread *td, char *upath, u_int64_t flags); diff --git a/sys/security/audit/audit_arg.c b/sys/security/audit/audit_arg.c index 32e317a..5da377f 100644 --- a/sys/security/audit/audit_arg.c +++ b/sys/security/audit/audit_arg.c @@ -357,13 +357,14 @@ audit_arg_process(struct proc *p) { struct kaudit_record *ar; + KASSERT(p != NULL, ("audit_arg_process: p == NULL")); + + PROC_LOCK_ASSERT(p, MA_OWNED); + ar = currecord(); - if ((ar == NULL) || (p == NULL)) + if (ar == NULL) return; - /* - * XXXAUDIT: PROC_LOCK_ASSERT(p); - */ ar->k_ar.ar_arg_auid = p->p_au->ai_auid; ar->k_ar.ar_arg_euid = p->p_ucred->cr_uid; ar->k_ar.ar_arg_egid = p->p_ucred->cr_groups[0]; @@ -404,21 +405,21 @@ audit_arg_socket(int sodomain, int sotype, int soprotocol) ARG_SET_VALID(ar, ARG_SOCKINFO); } -/* - * XXXAUDIT: Argument here should be 'sa' not 'so'. Caller is responsible - * for synchronizing access to the source of the address. - */ void -audit_arg_sockaddr(struct thread *td, struct sockaddr *so) +audit_arg_sockaddr(struct thread *td, struct sockaddr *sa) { struct kaudit_record *ar; + KASSERT(td != NULL, ("audit_arg_sockaddr: td == NULL")); + KASSERT(sa != NULL, ("audit_arg_sockaddr: sa == NULL")); + ar = currecord(); - if (ar == NULL || td == NULL || so == NULL) + if (ar == NULL) return; - bcopy(so, &ar->k_ar.ar_arg_sockaddr, sizeof(ar->k_ar.ar_arg_sockaddr)); - switch (so->sa_family) { + bcopy(sa, &ar->k_ar.ar_arg_sockaddr, + sizeof(ar->k_ar.ar_arg_sockaddr)); + switch (sa->sa_family) { case AF_INET: ARG_SET_VALID(ar, ARG_SADDRINET); break; @@ -428,7 +429,7 @@ audit_arg_sockaddr(struct thread *td, struct sockaddr *so) break; case AF_UNIX: - audit_arg_upath(td, ((struct sockaddr_un *)so)->sun_path, + audit_arg_upath(td, ((struct sockaddr_un *)sa)->sun_path, ARG_UPATH1); ARG_SET_VALID(ar, ARG_SADDRUNIX); break; @@ -472,17 +473,14 @@ audit_arg_text(char *text) { struct kaudit_record *ar; + KASSERT(text != NULL, ("audit_arg_text: text == NULL")); + ar = currecord(); if (ar == NULL) return; - /* - * XXXAUDIT: Why do we accept a possibly NULL string here? - */ /* Invalidate the text string */ ar->k_ar.ar_valid_arg &= (ARG_ALL ^ ARG_TEXT); - if (text == NULL) - return; if (ar->k_ar.ar_arg_text == NULL) ar->k_ar.ar_arg_text = malloc(MAXPATHLEN, M_AUDITTEXT, @@ -600,9 +598,10 @@ audit_arg_file(struct proc *p, struct file *fp) struct vnode *vp; int vfslocked; - /* - * XXXAUDIT: Why is the (ar == NULL) test only in the socket case? - */ + ar = currecord(); + if (ar == NULL) + return; + switch (fp->f_type) { case DTYPE_VNODE: case DTYPE_FIFO: @@ -618,14 +617,8 @@ audit_arg_file(struct proc *p, struct file *fp) break; case DTYPE_SOCKET: - ar = currecord(); - if (ar == NULL) - return; - - /* - * XXXAUDIT: Socket locking? Inpcb locking? - */ so = (struct socket *)fp->f_data; + SOCK_LOCK(so); if (INP_CHECK_SOCKAF(so, PF_INET)) { if (so->so_pcb == NULL) return; @@ -646,6 +639,7 @@ audit_arg_file(struct proc *p, struct file *fp) pcb->inp_lport; ARG_SET_VALID(ar, ARG_SOCKINFO); } + SOCK_UNLOCK(so); break; default: @@ -669,8 +663,12 @@ audit_arg_upath(struct thread *td, char *upath, u_int64_t flag) struct kaudit_record *ar; char **pathp; - if (td == NULL || upath == NULL) - return; /* nothing to do! */ + KASSERT(td != NULL, ("audit_arg_upath: td == NULL")); + KASSERT(upath != NULL, ("audit_arg_upath: upath == NULL")); + + ar = currecord(); + if (ar == NULL) + return; /* * XXXAUDIT: Witness warning for possible sleep here? @@ -680,10 +678,6 @@ audit_arg_upath(struct thread *td, char *upath, u_int64_t flag) KASSERT((flag != ARG_UPATH1) || (flag != ARG_UPATH2), ("audit_arg_upath: flag %llu", (unsigned long long)flag)); - ar = currecord(); - if (ar == NULL) - return; - if (flag == ARG_UPATH1) pathp = &ar->k_ar.ar_arg_upath1; else @@ -720,13 +714,10 @@ audit_arg_vnode(struct vnode *vp, u_int64_t flags) struct vattr vattr; int error; struct vnode_au_info *vnp; - struct thread *td; - /* - * XXXAUDIT: Why is vp possibly NULL here? - */ - if (vp == NULL) - return; + KASSERT(vp != NULL, ("audit_arg_vnode: vp == NULL")); + KASSERT((flags == ARG_VNODE1) || (flags == ARG_VNODE2), + ("audit_arg_vnode: flags %jd", (intmax_t)flags)); /* * Assume that if the caller is calling audit_arg_vnode() on a @@ -740,17 +731,10 @@ audit_arg_vnode(struct vnode *vp, u_int64_t flags) return; /* - * XXXAUDIT: KASSERT argument validity instead? - * * XXXAUDIT: The below clears, and then resets the flags for valid * arguments. Ideally, either the new vnode is used, or the old one * would be. */ - if ((flags & (ARG_VNODE1 | ARG_VNODE2)) == 0) - return; - - td = curthread; - if (flags & ARG_VNODE1) { ar->k_ar.ar_valid_arg &= (ARG_ALL ^ ARG_VNODE1); vnp = &ar->k_ar.ar_arg_vnode1; @@ -759,7 +743,7 @@ audit_arg_vnode(struct vnode *vp, u_int64_t flags) vnp = &ar->k_ar.ar_arg_vnode2; } - error = VOP_GETATTR(vp, &vattr, td->td_ucred, td); + error = VOP_GETATTR(vp, &vattr, curthread->td_ucred, curthread); if (error) { /* XXX: How to handle this case? */ return; @@ -786,10 +770,17 @@ audit_arg_vnode(struct vnode *vp, u_int64_t flags) void audit_sysclose(struct thread *td, int fd) { + struct kaudit_record *ar; struct vnode *vp; struct file *fp; int vfslocked; + KASSERT(td != NULL, ("audit_sysclose: td == NULL")); + + ar = currecord(); + if (ar == NULL) + return; + audit_arg_fd(fd); if (getvnode(td->td_proc->p_fd, fd, &fp) != 0) |