In a few perl scripts (that happen to be for macOS), we check whether
'$? >> 8' is zero to see whether a command failed. But the value of
'$? >> 8' is just the exit code of the process. If it instead, for
example, was killed by a signal, there is no exit code, and '$? >> 8'
may not be accurate (the terminating signal is in '$? & 127'). We should
check if $? is nonzero at all to see if an error happened.
To avoid any possible issues, update all of our checks for command
failure to check if $? is nonzero, instead of '$? >> 8'. In notarize.pl,
print out the whole status in addition to the exit code, just to be
clear in case the command somehow terminates from a signal.
Change-Id: I07ed145fae45d0bdc77c1249cf4d89bd5ba5cc56
Reviewed-on: https://gerrit.openafs.org/15980
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Marcio Brito Barbosa <mbarbosa@sinenomine.net>
Tested-by: Andrew Deason <adeason@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
To create the OpenAFS client for macOS, the current process involves
building the code, signing the binaries, creating the package, and
notarizing it. Each step is typically performed separately and requires
distinct parameters and credentials, making this process cumbersome and
difficult to follow.
To simplify this process, introduce the following '--with' options:
--with-macos-app-key
--with-macos-inst-key
--with-macos-keychain-profile
These options allow users to specify the credentials needed for signing
and notarizing the package upfront.
With these enhancements, users will be able to perform the entire
workflow - building, signing, creating, and notarizing the package -
with a single 'make packages' command, significantly simplifying this
process.
Change-Id: Ibf114f4f5bbe9bc72f37adc487c046e5243f5a97
Reviewed-on: https://gerrit.openafs.org/15977
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Tested-by: Andrew Deason <adeason@sinenomine.net>
At WWDC 2021, Apple introduced notarytool, a new utility for interacting
with the Apple notary service. Concurrently, Apple deprecated altool for
notarization purposes, with plans to discontinue its functionality on
November 1, 2023.
Currently, notarize.pl relies on the deprecated altool, which is no
longer supported. This commit updates this script to use notarytool,
ensuring it remains fully functional for notarizing the OpenAFS package.
Note that the arguments for notarize.pl have changed. Users can no
longer provide a plain password or keychain reference using the
@keychain prefix. Instead, a keychain profile for the credentials must
be created first, with the profile name provided as an argument.
This change is mandated by the new notarytool workflow.
Additionally, update pkgbuild.sh.in, as this script calls notarize.pl
if the --apple-id option is provided. Since notarize.pl now requires a
keychain profile name instead of an Apple ID and password, remove the
--apple-id option from pkgbuild.sh.in and introduce a new option,
--keychain-profile, which specifies the name of the keychain profile to
be used by notarize.pl.
Change-Id: I8b513e3eebb38e49f0d7c5ff9ea4e1d46e87aa3f
Reviewed-on: https://gerrit.openafs.org/15976
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Since commit f7ddab6606 (Merge pam into the kauth configure option), we
set enable_pam="yes" if --enable-kauth was given to configure. That
makes sense, but we also do that if --disable-kauth was given to
configure. This doesn't make any sense, since we disable enable_pam and
enable_kauth if no --enable-kauth/--disable-kauth option was given at
all.
This doesn't affect much, since INSTALL_KAUTH will be disabled for
--disable-kauth, so the items in src/pam won't be installed in this case
regardless. But to make our behavior more consistent, and actually avoid
kauth stuff for --disable-kauth, set enable_pam to the same thing as
enable_kauth when an option is given, so pam is disabled when kauth is
disabled.
Change-Id: Ib36e9fdffb93d659e1fadab5f1bb1334770806b8
Reviewed-on: https://gerrit.openafs.org/15995
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Several build artifacts are missing from the gitignore rules when
building on macOS. For example:
$ git status --porcelain
?? src/libafs/afs.x86_darwin_210.plist
?? src/platform/DARWIN/PrivilegedHelper/org.openafs.privhelper
Update the gitignore rules to ignore generated darwin plist files and
the prefpane "privileged helper" binary.
Change-Id: I684ec9edde3da76cc83a644f0150da800f6f3db5
Reviewed-on: https://gerrit.openafs.org/16002
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Marcio Brito Barbosa <mbarbosa@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
OpenAFS builds both shared and static libraries by default. On most
platforms, each library is built as a shared library libfoo.so.x.y.z,
and as a static library libfoo.a. libtool refers to this as the 'svr4'
style of libraries on AIX.
On AIX, traditionally libraries are built with the shared library,
static library, and various versions and 64-bit variants and such all
contained in the same libfoo.a. libtool refers to this as the 'aix'
style of libraries on AIX.
libtool defaults to the 'aix' style on AIX, and so this is how libtool
attempts to build our libraries on AIX. For many of our libraries, even
though the build completes successfully, this results in broken
binaries because our Makefile rules assume the shared and static library
files have different names.
For example, here is the install rule for librokenafs:
install: $(SHARED_LIBS) librokenafs.a
$(LT_INSTALL_DATA) librokenafs.la $(DESTDIR)$(libdir)/librokenafs.la
$(RM) $(DESTDIR)$(libdir)/librokenafs.la
$(INSTALL_DATA) librokenafs.a $(DESTDIR)$(libdir)/librokenafs.a
On AIX, the LT_INSTALL_DATA step will install the shared library
librokenafs.a into $(DESTDIR)$(libdir). Then the INSTALL_DATA step will
install the static library librokenafs.a into the same location,
deleting the shared library file.
When the user tries to run an executable, they get an error, because the
shared library librokenafs is not installed at all:
$ /opt/openafs/bin/pts
exec(): 0509-036 Cannot load program /opt/openafs/bin/pts because of the following errors:
0509-150 Dependent module /opt/openafs/lib/librokenafs.a(librokenafs.so.2) could not be loaded.
0509-152 Member librokenafs.so.2 is not found in archive
To avoid this, the user can run configure with --with-aix-soname=svr4 to
build all of our shared libraries in the 'svr4' style. In the above
example, this causes LT_INSTALL_DATA to install the shared librokenafs
as librokenafs.so.* into $(DESTDIR)$(libdir), and then the static
library is installed as librokenafs.a like normal. The resulting
binaries can then run without issue.
To make it so users don't need to specify --with-aix-soname=svr4 to get
a working build, change the default behavior to --with-aix-soname=svr4
by passing aix-soname=svr4 to LT_INIT.
However, just specifying LT_INIT([aix-soname=svr4]) alone does not work,
due to a bug in libtool: https://savannah.gnu.org/support/?111161
To workaround this bug, also explicitly turn on shared and static
libraries by default in LT_INIT, even though they are already on by
default. Using --disable-shared or --disable-static should still be
honored as normal; the LT_INIT arguments just specify the default
values.
Ideally, we would install our shared libraries in the 'aix' style, so a
single libfoo.a contains all of the necessary .o and .so files for that
library. However, changing our build system to do this is difficult. And
historically, OpenAFS has built shared libraries on AIX in the 'svr4'
style; that is how we built shared libraries in OpenAFS 1.6 and earlier,
before we converted to using libtool.
Change-Id: Ifd2538c635323c568f0d8b2e621cc0b8594721ae
Reviewed-on: https://gerrit.openafs.org/15983
Reviewed-by: Ben Huntsman <ben@huntsmans.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
When building the libafs kernel module, we pass various flags to the
compiler and linker, some of which no longer work with the clang-based
Open XL C 17.1 and newer:
On XL C 16.1 and below, the -M argument causes a file to be generated
with a listing of all the files upon which the .o is dependent. This
file is created with the same name as the .o file, but with a .u
extension. With the Open XL C 17.1+ compilers, the -M file acts
similarly, but if -o is specified, the dependency file is created
with the name specified by the -o argument. This causes the
dependency file to be created but not the actual object code file.
Because the compiler invocation succeeds, this repeats until it is
time to link, which fails because the .o files are not actually code,
but ascii files containing the dependency information. We don't
actually use the .u files for anything, so to avoid this issue, remove
the -M flag.
The -H8 flag is a linker argument that aligns the text, data, and loader
sections of the output file so that each section begins on a file offset
that is a multiple of 8. On XL C 16.1 and below, the compiler passes
unrecognized arguments to the linker, and so giving -H8 to the compiler
works fine. But with XL C 17.1+, the compiler does not pass unrecognized
arguments to the linker, and throws an error instead. To avoid this,
move -H8 from KDEFS to LDFLAGS, so it's given only to the linker and not
to the compiler.
The -q64 flag no longer exists with XL C 17.1+, and is replaced by -m64.
Use ${XCFLAGS64} instead of -q64, which will choose the correct flag to
use, like we did in commit aa82a8894f (export: Use XCFLAGS64 for -q64).
Change-Id: I2527a604000507ec27adb37338d98a6852ad3131
Reviewed-on: https://gerrit.openafs.org/15974
Reviewed-by: Ben Huntsman <ben@huntsmans.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
clang-19 is flagging a warning/error in rxkad/test/stress.c as
unused-but-set-variables
stress.c:51:9: error: variable 'nonoauth' set but not used [-Werror,-Wunused-but-set-variable]
51 | int nonoauth = 0;
| ^
1 error generated.
The variable nonoauth is initialized and conditionally incremented, but
is not used elsewhere.
Remove the nonoauth variable.
Change-Id: I9a97a3980e5b6ff462f860d7bdae4ee8d7b22971
Reviewed-on: https://gerrit.openafs.org/15984
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Convert the prefpane logic for writing to config files to use
privhelper, so it can work with macOS 10.8+. With this commit, the
"CellServDB Editor" and "Parameter" tabs in the prefpane should now
work.
Specifically, define the privhelper task "write". Define a new variant
to TaskUtil's executePrivTask() called executePrivTaskWrite() to use the
new task type, and convert the prefpane code to use it.
Most files are straightforward, but converting writeAfsdOption() is a
little awkward due to the repeated checking of useAfsdConfVersion. Just
remove the writeAfsdOption() method and make the caller choose between
writeNewAfsdOption() and writeOldAfsdOption().
The new "write" task takes a new argument (data) to indicate what data
to write to the config file in question. Like the "backup" task, it also
takes a "filename" argument that is checked against our list of config
files.
Change-Id: Ib93da5a75a92e3507c15f9ee03eb8f7fbad7d088
Reviewed-on: https://gerrit.openafs.org/15960
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Convert the prefpane logic to backup various config files to privhelper,
so it can work with macOS 10.8+.
Specifically, define the new privhelper task "backup". Define a new
variant to TaskUtil's executePrivTask() called executePrivTaskBackup()
to use the new task type, and convert the prefpane code to use it.
Remove the backupFile() method, since it's now unused.
The new "backup" task has an argument (filename), so we don't need to
define separate tasks for each file we want to backup. But check the
given filename against a short list of files we know we deal with, just
to make sure we don't accidentally mess with some other system file,
since we're running as root.
Change-Id: I066d8e95a50a52f7509487f9228ba03dd027ea36
Reviewed-on: https://gerrit.openafs.org/15959
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Convert the logic to "start/stop OpenAFS" in the prefpane to use
privhelper, so it can work with macOS 10.8+.
Specifically, define these new privhelper tasks:
- afsd_start
- afsd_stop
And convert our start/stop-related code in the prefpane to use them.
Change-Id: I4cc5b500fb09b83a4007e2c7c5045e93fe1f4ced
Reviewed-on: https://gerrit.openafs.org/15958
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Convert the logic for the "start at login" option in the prefpane to use
privhelper, so it can work with macOS 10.8+.
Specifically, define these new privhelper tasks:
- startup_enable
- startup_disable
- startup_check
And convert our startup-related logic in the prefpane to use them.
Get rid of our now-unused method executeTaskWithAuth() and related
methods.
Change-Id: I2cb4c31f964529ab1af43ab7828c14eba7354af0
Reviewed-on: https://gerrit.openafs.org/15957
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
The prefpane for macOS runs as the logged-in user, but needs root access
for some operations: starting/stopping the client, editing various
configuration files like CellServDB, etc. We currently use functions
like AuthorizationExecuteWithPrivileges() to run commands with root
privileges directly, but this approach no longer works as of macOS 10.8
(Mountain Lion); the relevant functions have been removed.
Instead, a new approach exists as of macOS 10.6 (Snow Leopard). The
prefpane application itself cannot gain root privileges, but we can
provide another daemon process that runs as root, and the PrefPane sends
requests to that process to perform the privileged operations we need.
In this commit, create a separate helper program called PrivilegedHelper
(privhelper for short) that serves this purpose. Define the
executePrivTask() method in TaskUtil to handle communicating with
privhelper over XPC.
This commit does not define any of the tasks that privhelper will
actually perform; this just implements privhelper itself. Later commits
will add and use various privileged tasks in privhelper.
In order for privhelper to be able to run as root, both privhelper and
the prefpane itself must be code signed and the relevant apple team id
must be specified in their Info.plist when they are built, as well as
inside privhelper.c. Currently, we have
no way of specifying code signatures info during the build, since all code
signing is done when generating packages (via pkgbuild.sh) after
binaries are built. For now, just put a commented-out section in
src/platform/DARWIN/AFSPreference/Info.plist and
src/platform/DARWIN/PrivilegedHelper/privhelper-info.plist and a
placeholder in src/platform/DARWIN/PrivilegedHelper/privhelper.c to show
how to add this information. The package builder must add their own team
id to these before privhelper can work properly.
The privhelper tool checks that the calling user has authorization to
run commands as root (via AuthorizationCopyRights()), and that the
calling process is either our AFSBackgrounder menu bar or the prefpane.
We use xpc_connection_set_peer_code_signing_requirement() for this where
available, but fallback to using SecCodeCheckValidity() with
SecCodeCreateWithXPCMessage() or
xpc_dictionary_get_audit_token()/SecCodeCopyGuestWithAttributes() if
needed.
Change-Id: I724b6d486ee5397c89c79e589ddcb2a5987a895b
Reviewed-on: https://gerrit.openafs.org/15956
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
The default Xcode compiler options are insufficient to fail the build if
a method is not found. For example, a typo in the name of method
'componentsJoinedByString' results in the following build warning:
/TaskUtil.m:70:62: warning: instance method '-componentsJoinedBySting:'
not found (return type defaults to 'id') [-Wobjc-method-access]
NSLog(@"Task failed: %@ %@ status:%d.", taskName,
[args componentsJoinedBySting:@", "], status);
Because this is only flagged as a warning, the build completes
successfully. When this code runs, the AFSBackgrounder merely logs a
runtime exception every time it passes through the erroneous code. This
is a silent failure, unless you happen to know how to check for
AFSBackgrounder log messages:
$ log show --predicate 'process=="AFSBackgrounder"'
Add compiler flag '-Werror=objc-method-access' to all of our xcode-built
project files to treat this case as an error instead of a warning, so
the build will fail.
[adeason@sinenomine.net: Update all xcode project files.]
Change-Id: I69a9f45deb6710a50590bd79daf07466332a6ad1
Reviewed-on: https://gerrit.openafs.org/14586
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Commit 91378d93b9 (rx: Don't send packets to localhost if -rxbind set)
added the rxi_GetLocalAddr() function, but missed the prototype. Add the
function prototype to fix a GCC missing-prototype warning when building
the linux kernel module.
Fixes the build error when building the kernel module for Linux 6.8
or later when the tree was configured with --enable-checking:
.../src/libafs/MODLOAD-6.8.0-49-generic-SP/rx.c:9693:1: error: no
previous prototype for ‘rxi_GetLocalAddr’ [-Werror=missing-prototypes]
9693 | rxi_GetLocalAddr(struct sockaddr_in *sin)
| ^~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
Change-Id: I43bd56fa28d258be509b1d1381e2f7d76ad5a532
Reviewed-on: https://gerrit.openafs.org/15978
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Tested-by: Andrew Deason <adeason@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Tested-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
The comments above VCHash claim that PFlush relies on the fid uniq not
being used in VCHash, but this doesn't appear to be true.
However, we do have some code that relies on VCHash not using uniq:
afs_GenFakeFid. Update the VCHash comment to point this out.
Change-Id: Ib1e0c1700c4cd13b95ac57522645ce73d2b24a08
Reviewed-on: https://gerrit.openafs.org/15042
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
For some platforms (SOLARIS, FBSD), shutting down the libafs client
currently involves sending a packet to localhost to wakeup the listener
thread. If rx is bound to a specific host address (the -rxbind option
was passed to afsd), this won't work, because rx won't receive packets
sent to localhost. This results in the client hanging forever when
trying to 'umount afs', until a packet is otherwise sent to the rx
socket.
To fix this, send the packet to the bound host address instead, if
-rxbind was given. Otherwise, send the packet to localhost, like before.
Introduce the small helper function rxi_GetLocalAddr() to consolidate
the logic of what address to use.
Change-Id: I26fa5194b726ed753779faa07142fec647228b44
Reviewed-on: https://gerrit.openafs.org/15906
Reviewed-by: Marcio Brito Barbosa <mbarbosa@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Tested-by: Andrew Deason <adeason@sinenomine.net>
Remember what local address we're bound to (as passed to rx_InitHost(),
which is given when -rxbind is passed to various programs), like we do
for our local port with rx_port. Store this in a new internal global
variable, rx_host.
This commit just introduces the new variable, but does not use it.
Future commits will add code that make use of this information.
Change-Id: I5ac1a60b10f6c28df1e6620e30515bcdfd749311
Reviewed-on: https://gerrit.openafs.org/15905
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Marcio Brito Barbosa <mbarbosa@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
The indexGreaterThanIndex function returns the closest index from the
index set that is greater than a specified index. This function is
typically used in a loop to iterate through a list of items until it
returns NSNotFound, indicating the end of the list.
In AFSCommanderPref.m, the following pattern is being used:
int index = 0;
do {
...
} while((index = [... indexGreaterThanIndex:index]) != NSNotFound);
The issue arises because indexGreaterThanIndex returns an NSUInteger,
while the loop uses an int for index. If NSNotFound is cast to an int,
it becomes -1, causing the loop to never terminate and leading to a
crash.
To fix this problem, change the type of index from int to NSUInteger to
ensure proper comparison and termination of the loop when NSNotFound is
returned.
Change-Id: I0b8b38d201b500c2d6d7778f0132ce17f7c04ac8
Reviewed-on: https://gerrit.openafs.org/15961
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Address potential memory issues by setting workIPArray to nil after
releasing it in commitModify. This prevents dangling pointers and
ensures consistency with rollbackModify, which already includes this
safeguard. Without this change, PrefPane can crash upon closing the
window used for setting IP addresses of cells in the CellServDB.
Change-Id: Iea56343d734f854b76b74c961d2df05bcf4a9332
Reviewed-on: https://gerrit.openafs.org/15962
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
On FreeBSD, we commonly generate some kernel header files in
src/libafs/kconf-GENERIC, ever since commit a9c1939eeb (FBSD: Use
GENERIC kernel headers by default). Ignore it.
Change-Id: I2ce14361b130f20861133a8358aaa4085073e897
Reviewed-on: https://gerrit.openafs.org/15902
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Ben Huntsman <ben@huntsmans.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Most of our binaries link with -lhcrypto or our internally-built
-lafshcrypto for heimdal-provided crypto code. But we also build an LWP
variant, libafshcrypto_lwp.a, which avoids any pthread calls and is to
be used when linking against LWP instead of pthreads.
Currently, a few binaries link against LWP, but also link against
$(LIB_hcrypto) (which gets expanded to -lhcrypto or -lafshcrypto). This
includes most binaries in src/tests, src/rx/test, and src/rxkad/test,
and has been the case since these commits:
236cb51b83 rx: Cleanup and build src/rx/test
1fe1dac4c5 rxkad: Cleanup and build src/rxkad/test
80c23d958c tests: Make src/tests buildable
On most platforms, this isn't so noticeable; the binaries just get built
for LWP, but have a pthreaded hcrypto and contain references to
pthreads. (hcrypto itself just uses a single global mutex that is
briefly held, and so tends to not cause cause any problems running with
LWP.)
On AIX, this causes a build error when building with --disable-shared,
because we build these programs without linking to the pthreads library:
/opt/IBM/xlC/16.1.0/bin/cc [...] -o test-setgroups test-setgroups.o [...]/lib/libopr.a -lafshcrypto -lrokenafs
ld: 0711-317 ERROR: Undefined symbol: .pthread_mutex_lock
ld: 0711-317 ERROR: Undefined symbol: .pthread_mutex_unlock
ld: 0711-345 Use the -bloadmap or -bnoquiet option to obtain more information.
make: *** [test-setgroups] Error 8
(Without --disable-shared, this succeeds, presumably because we link
against the libtool libafshcrypto.la library, which is smart enough to
know it depends on pthreads.)
To fix this, link against libafshcrypto_lwp.a for these LWP binaries
instead of $(LIB_hcrypto), like other LWP binaries do. We could maybe
change $(LIB_hcrypto) to expand to libafshcrypto_lwp.a for Makefile.lwp,
but that can be confusing, especially for directories that build both
LWP and pthreaded binaries.
Written in collaboration with ben@huntsmans.net, who noticed the issue
and provided testing.
Change-Id: Ic4eef01c40e3ecdc4a8dc999b21273d7da364d34
Reviewed-on: https://gerrit.openafs.org/15904
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Ben Huntsman <ben@huntsmans.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
scan-build identified these as unused operations that can be removed.
Change-Id: I6fe7c3c23cfcbecbe8fa539131aec779c42876c2
Reviewed-on: https://gerrit.openafs.org/13305
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Several static analysis tools have identified various problems:
- files left open from early returns/exits (infer)
- possible memory leaks (scan-build, cppcheck)
To resolve the above problems:
- close files before returning/exiting
- fix possible memory leaks by freeing memory
This commit is a reorganization of commits developed by Pat Riehecky,
who ran the static analysis tools and developed the fixes.
Change-Id: Id8d25bacd2d1e1dee32354248519e3821ffe726c
Reviewed-on: https://gerrit.openafs.org/14681
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Several static analysis tools have identified various problems:
- possible memory leaks (scan-build, cppcheck)
To resolve the above problems:
- fix possible memory leaks by freeing memory
This commit is a reorganization of commits developed by Pat Riehecky,
who ran the static analysis tools and developed the fixes.
Change-Id: Ica9e5a33bf33ae802fed0f1439fc3561d0406713
Reviewed-on: https://gerrit.openafs.org/14685
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Reviewed-by: PatRiehecky <jcpunk@gmail.com>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Several static analysis tools have identified various problems:
- use of uninitlized variables (scan-build)
- missing checks to ensure *alloc was successful (infer)
- possible memory leaks (scan-build, cppcheck)
To resolve the above problems:
- ensure variables are initialized to safe defaults
- add checks to ensure *alloc was successful before using the memory
- fix possible memory leaks by freeing memory
This commit is a reorganization of commits developed by Pat Riehecky,
who ran the static analysis tools and developed the fixes.
Change-Id: Ic5e222e344df4d5a3e317fcbb2f30a09dce93793
Reviewed-on: https://gerrit.openafs.org/14679
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Several static analysis tools have identified various problems:
- missing checks to ensure *alloc was successful (infer)
To resolve the above problems:
- Add asserts to verify that the *alloc was successful before using the
memory.
This commit is a reorganization of commits developed by Pat Riehecky,
who ran the static analysis tools and developed the fixes.
Note: Most of the callers to cmd_AddParmAlias and cmd_OptionAsList do
not check the returned value, so asserts are used instead of returning
ENOMEM.
Change-Id: Ic44f6d34240c13c2626567f9d698d2480d2e8672
Reviewed-on: https://gerrit.openafs.org/14680
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Several static analysis tools have identified various problems:
- printf conversion specifiers (cppcheck)
- sscanf string overflows (cppcheck)
- files left open from early exits (infer)
- dereference of NULL pointer (scan-build)
- missing checks to ensure *alloc was successful (infer)
- possible memory leaks (scan-build, cppcheck)
To resolve the above problems:
- update printf conversion specifiers
- update sscanf format strings to include the size of the buffer
- close files before exiting
- ensure pointers are non-NULL prior to use
- add checks to ensure *alloc was successful before using the memory
- fix possible memory leaks by freeing memory
This commit is a reorganization of commits developed by Pat Riehecky,
who ran the static analysis tools and developed the fixes.
Change-Id: I864b2c6be0c7dec1e93b49231ad7876d38c20435
Reviewed-on: https://gerrit.openafs.org/14677
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Clang's scan-build static analysis tool flagged a 'Use of memory after
it is freed'
The function _DirUpdate can reallocate the storage that is pointed to
by the dirbuffer member of the DirHeader structure.
The code in afscp_DirLookup initializes a local variable to d->dirbuffer
before calling _DirUpdate.
Update afscp_dir.c to initialize the local variable 'h' after calling
_DirUpdate.
Change-Id: Ibfc8b25ac31998f8581185f896737ca4bc27da7e
Reviewed-on: https://gerrit.openafs.org/14713
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
In the project root .gitignore, fix a typo which was causing the
built products directory to not be properly ignored.
Change-Id: Iea83a135b6f5a4c60c76df7c8bfcc65df90e1b39
Reviewed-on: https://gerrit.openafs.org/15903
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Several static analysis tools have identified various problems:
- missing checks to ensure *alloc was successful (infer)
To resolve the above problems:
- add checks to ensure *alloc was successful before using the memory
This commit is a reorganization of commits developed by Pat Riehecky,
who ran the static analysis tools and developed the fixes.
Change-Id: I48e053e73131c3f86928a4658dbc69e56abd2b20
Reviewed-on: https://gerrit.openafs.org/14683
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Currently, if the dynamic vcaches feature is enabled, the cache manager
deallocates batches of vcaches (allocated on demand) in the background.
Since this procedure is executed once every 5 minutes (at most), the
number of vcaches can grow unlimitedly between executions, increasing
the cost of using and maintaining them during this timeframe.
To prevent this from happening, run afs_ShakeLooseVCaches() sooner if we
have way more vcaches than the configured threshold.
Change-Id: I10ce7487441c659463ec85b4c8390274c11ef43f
Reviewed-on: https://gerrit.openafs.org/15044
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
On some platforms (such as LINUX), unlinked vcaches are not destroyed
until the next execution of afs_ShakeLooseVCaches(). Since the hash for
a given vcache is computed from its volume id and vnode number, keeping
unlinked vcaches around can be a problem if files are repeatedly deleted
and recreated in the same volume. In this scenario, the same vnode
number will be reused several times, resulting in many vcaches with the
same volume id and vnode number (but different unique ids) in the same
bucket. Consequently, finding a given vcache in the bucket in question
can be time consuming (and cause extra cpu processing), as we would have
to traverse a long linked list.
To mitigate this problem, move vcaches associated with unlinked files to
the least recently used position in our VLRU, prioritizing their removal
the next time afs_ShakeLooseVCaches() is executed.
Introduce the QAddEnd() macro to make it easier to add a vcache to the
end of the VLRU.
Change-Id: Idc74da0c3c0b27bfdf2cd491b003bdd42c889a95
Reviewed-on: https://gerrit.openafs.org/14961
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Linux-6.12 commit 'fs: Convert aops->write_begin to take a folio'
(1da86618bdce3) changed the address_space_operations's members
write_begin and write_end to use a folio instead of a page.
Add configure check to test the signature for aop's write_begin and
write_end members to see if they take a folio parameter.
Update the afs_linux_write_begin and afs_linux_write_end functions to
use a folio instead of a page.
Change-Id: Idd38aa3a9871e188580aae86f5b22621a5511bb2
Reviewed-on: https://gerrit.openafs.org/15898
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
The function afs_linux_write_begin() has 2 preprocessor selected
implementations, one to handle the case where write_begin has a flag
parameter and the other where it doesn't.
Refactor the code to combine the 2 implementations using preprocessor
conditionals for the function declaration and within the body of the
function as needed.
There are no functional changes.
This refactoring is in preparation for additional changes that will be
made to the afs_linux_write_begin() function.
Change-Id: I5389ea562d834d847bc7413e6f58dc3be2e5aa31
Reviewed-on: https://gerrit.openafs.org/15897
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Marcio Brito Barbosa <mbarbosa@sinenomine.net>
The Linux 6.12 commit 'mm: remove PG_error' (09022bc196d23) removed
the PG_error page flag and the associated ClearPageError() and
SetPageError() functions (via removing the PAGEFLAG(Error, ...) macro).
The PG_error flag has not been used by core VFS/MM Linux code for some
time, possibly ever, and so our calls to these functions do not have any
practical effect, since we also do not check for the PG_error flag.
While we could simply remove these calls, play it safe and keep them
around until ClearPageError()/SetPageError() are removed.
The specific semantics of the PG_error flag are not completely well
defined in the Linux kernel, which appears to be one of the motivations
for its removal. Some Linux commits that mention some details on how the
flag is not useful for read errors include:
7edf1ec5b249 ceph: don't SetPageError on readpage errors
41a638a1b3fc affs: convert affs_symlink_read_folio() to use the folio
2b2553f12355 btrfs: stop setting PageError in the data I/O path
Add a configure test to see if ClearPageError()/SetPageError() are
available in the Linux kernel; if they are not, define
ClearPageError()/SetPageError() as no-ops.
Change-Id: I2d3235d81030aa0b30907336f58a7d9c5c7b8026
Reviewed-on: https://gerrit.openafs.org/15876
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Tested-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Marcio Brito Barbosa <mbarbosa@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Commit 1f5e1ef9e3 (OPENAFS-SA-2024-003: Run xdr_free for retried RPCs)
added a couple of references to xdr_namelist, which currently causes a
build failure on AIX:
/bin/sh ../../libtool --quiet --mode=link --tag=CC xlc_r [...] -o pts pts.o ../../src/ptserver/liboafs_prot.la [...]
ld: 0711-317 ERROR: Undefined symbol: xdr_namelist
ld: 0711-345 Use the -bloadmap or -bnoquiet option to obtain more information.
make: 1254-004 The error code from the last command is 8.
To avoid this, add xdr_namelist to liboafs_prot.la.sym.
Change-Id: I8c9a7a64efdb9107e27a3b3502e5d96272f5341e
Reviewed-on: https://gerrit.openafs.org/15954
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Update version strings for the third 1.9.x development release.
Change-Id: I203a9d3487d1344882afb02ded094e01e7672753
Reviewed-on: https://gerrit.openafs.org/15926
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
CVE-2024-10397
Currently, there are a few callers of RPCs that specify some data for
an INOUT parameter, but do not initialize the memory for that data.
This can result in the uninitialized memory being sent to the peer
when the argument is processed as an IN argument. Simply clear the
relevant data before running the RPC to avoid this.
The relevant RPCs and arguments are:
- For RMTSYS_Pioctl, the 'OutData' argument.
- For BUDB_GetVolumes, the 'volumes' argument.
-- via DBLookupByVolume -> bcdb_LookupVolume -> ubik_BUDB_GetVolumes
-- and via bc_Restorer -> bcdb_FindVolumes -> ubik_BUDB_GetVolumes
- For KAA_Authenticate_old / KAA_Authenticate, this can happen with
the 'answer' argument in ka_Authenticate if KAA_AuthenticateV2 or
KAA_Authenticate return RXGEN_OPCODE, but the server manages to
populate oanswer.SeqLen with non-zero.
For all of these, make sure the memory is blanked before running the
relevant RPC. For ka_Authenticate, reset oanswer.SeqLen to 0 to avoid
sending any data, but still blank 'answer' and 'answer_old' just to be
safe.
FIXES 135043
Change-Id: I95ee91a1710d66e2e88c3086d57c279a376f7dc6
Reviewed-on: https://gerrit.openafs.org/15925
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
CVE-2024-10397
Here, 'OutData' only has OutData.rmtbulk_len bytes in it. We know that
OutData.rmtbulk_len is at most data->out_size, but it could be
smaller. So, only copy OutData.rmtbulk_len bytes, not data->out_size,
since data->out_size could be more than the number of bytes we have
allocated in OutData.
FIXES 135043
Change-Id: I6f87fc8cb5df0298061f419112200f6c7e1974ba
Reviewed-on: https://gerrit.openafs.org/15924
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
CVE-2024-10397
A few areas of code retry the same RPC, like so:
do {
code = VL_SomeRPC(rxconn, &array_out);
} while (some_condition);
xdr_free((xdrproc_t) xdr_foo, &array_out);
Or try a different version/variant of an RPC (e.g.
VLDB_ListAttributesN2 -> VLDB_ListAttributes).
If the first RPC call causes the output array to be allocated with
length N, then the subsequent RPC calls may fail if the server
responds with an array larger than N.
Furthermore, if the subsequent call responds with an array smaller
than N, then when we xdr_free the array, our length will be smaller
than the actual number of allocated elements. That results in two
potential issues:
- We'll fail to free the elements at the end of the array. This is
only a problem if each element in the array also uses
dynamically-allocated memory (e.g. each element contains a string or
another array). Fortunately, there are only a few such structures in
any of our RPC-L definitions: SysNameList and CredInfos. And neither
of those are used in such a retry loop, so this isn't a problem.
- We'll give the wrong length to osi_free when freeing the array
itself. This only matters for KERNEL, and only on some platforms
(such as Solaris), since the length given to osi_free is ignored
everywhere else.
To avoid these possible issues, change the relevant retry loops to
free our xdr-allocated arrays on every iteration of the loop, like
this:
do {
xdr_free((xdrproc_t) xdr_foo, &array_out);
code = VL_SomeRPC(rxconn, &array_out);
} while (some_condition);
xdr_free((xdrproc_t) xdr_foo, &array_out);
Or like this:
do {
code = VL_SomeRPC(rxconn, &array_out);
xdr_free((xdrproc_t) xdr_foo, &array_out);
} while (some_condition);
FIXES 135043
Change-Id: I4e058eb52d277e6d3817df8b703c5e5b894c0b5a
Reviewed-on: https://gerrit.openafs.org/15923
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
CVE-2024-10397
Currently, if a caller calls an RPC with a string output argument,
like so:
{
char *str = NULL;
code = RXAFS_SomeCall(&str);
/* do something with 'str' */
xdr_free((xdrproc_t) xdr_string, &str);
}
Normally, xdr_free causes xdr_string to call osi_free, specifying the
same size that we allocated for the string. However, since we only
have a char*, the amount of space allocated for the string is not
recorded separately, and so xdr_string calculates the size of the
buffer to free by using strlen().
This works for well-formed strings, but if we fail to decode the
payload of the string, or if our peer gave us a string with a NUL byte
in the middle of it, then strlen() may be significantly less than the
actual allocated size. And so in this case, the size given to osi_free
will be wrong.
The size given to osi_free is ignored in userspace, and for KERNEL on
many platforms like Linux and DARWIN. However, it is notably not
ignored for KERNEL on Solaris and some other less supported platforms
(HPUX, Irix, NetBSD). At least on Solaris, an incorrect size given to
osi_free can cause a system panic or possibly memory corruption.
To avoid this, change xdr_string during XDR_DECODE to make sure that
strlen() of the string always reflects the allocated size. If we fail
to decode the string's payload, replace the payload with non-NUL bytes
(fill it with 'z', an arbitrary choice). And if we do successfully
decode the payload, check if the strlen() is wrong (that is, if the
payload contains NUL '\0' bytes), and fail if so, also filling the
payload with 'z'. This is only strictly needed in KERNEL on certain
platforms, but do it everywhere so our behavior is consistent.
FIXES 135043
Change-Id: I90c419a7ef0ede247187172a182863dcb4250578
Reviewed-on: https://gerrit.openafs.org/15922
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
CVE-2024-10397
Various RPCs return a variable-length array in an OUT argument, but
are only supposed to return specific sizes. A few instances of this
include the following (but this is not an exhaustive list):
- AFSVolListOneVolume should only return a single volintInfo.
- PR_NameToID should return the same number of IDs as names given.
- VL_GetAddrsU should return the same number of addresses as the
'nentries' OUT argument.
Some callers of these RPCs just assume that the server has not
violated these rules. If the server responds with a nonsensical array
size, this could cause us to read beyond the end of the array, or
cause a NULL dereference or other errors.
For example, some callers of VL_GetAddrsU will iterate over 'nentries'
addresses, even if the 'blkaddrs' OUT argument contains fewer entries.
Or with AFSVolListOneVolume, some callers assume that at least 1
volintInfo has been returned; if 0 have been returned, we can try to
access a NULL array.
To avoid all of this, add various sanity checks on the relevant
returned lengths of these RPCs. For most cases, if the lengths are not
sane, return an internal error from the appropriate subsystem (or
RXGEN_CC_UNMARSHAL if there isn't one). For VL_GetAddrsU, if
'nentries' is too long, just set it to the length of the returned
array.
FIXES 135043
Change-Id: Ibdc7837ab09b4765436fc4c0d780e695bba07128
Reviewed-on: https://gerrit.openafs.org/15921
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
CVE-2024-10397
When making an RPC call from a client, output arguments that use
arrays (or array-like objects like strings and opaques) can be
allocated by XDR, like so:
{
struct idlist ids;
ids.idlist_val = NULL;
ids.idlist_len = 0;
code = PR_NameToID(rxconn, names, &ids);
/* data inside ids.idlist_val[...] */
xdr_free((xdrproc_t) xdr_idlist, &ids);
}
With this approach, during XDR_DECODE, xdr_array() reads in the number
of array elements from the peer, then allocates enough memory to hold
that many elements, and then reads in the array elements.
Alternatively, the caller can provide preallocated memory, like so:
{
struct idlist ids;
afs_int32 ids_buf[30];
ids.idlist_val = ids_buf;
ids.idlist_len = 30;
code = PR_NameToID(rxconn, names, &ids);
/* data inside ids.idlist_val[...] */
}
With this approach, during XDR_DECODE, xdr_array() reads in the number
of array elements from the peer, and then reads in the array elements
into the supplied buffer. However, in this case, xdr_array() never
checks that the number of array elements will actually fit into the
supplied buffer; the _len field provided by the caller is just ignored.
In this example, if the ptserver responds with 50 elements for the 'ids'
output argument, xdr_array() will write 50 afs_int32's into
'ids.idlist_val', going beyond the end of the 30 elements that are
actually allocated.
It's also possible, and in fact very easy, to use xdr-allocated
buffers and then reuse them as a preallocated buffer, possibly
accidentally. For example:
{
struct idlist ids;
ids.idlist_val = NULL;
ids.idlist_len = 0;
while (some_condition) {
code = PR_NameToID(rxconn, names, &ids);
}
}
In this case, the first call to PR_NameToID can cause the buffer for
'ids' to be allocated by XDR, which will then be reused by the
subsequent calls to PR_NameToId. Note that this can happen even if the
first PR_NameToID call fails; the call can be aborted after the output
array is allocated.
Retrying an RPC in this way is effectively what all ubik_Call*
codepaths do (including all ubik_* wrappers, e.g. ubik_PR_NameToID).
Or some callers retry effectively the same RPC when falling back to
earlier versions (e.g. VL_ListAttributesN2 -> VL_ListAttributesN).
To prevent this for arrays and opaques, change xdr_array (and
xdr_bytes) to check if the _len field for preallocated buffers is
large enough, and return failure if it's not.
Also perform the same check for the ka_CBS and ka_BBS structures. These
are mostly the same as opaques, but they have custom serialization
functions in src/kauth/kaaux.c. ka_BBS also has two lengths: the actual
length of bytes, and a 'max' length. ka_CBS isn't used for any RPC
output arguments, but fix it for consistency.
For strings, the situation is complicated by the fact that callers
cannot pass in how much space was allocated for the string, since
callers only provide a char**. So for strings, just refuse to use a
preallocated buffer at all, and return failure if one is provided.
Note that for some callers using preallocated arrays or strings, the
described buffer overruns are not possible, since the preallocated
buffers are larger than the max length specified in the relevant
RPC-L. For example, afs_DoBulkStat() allocates AFSCBMAX entries for
the output args for RXAFS_InlineBulkStatus, which is the max length
specified in the RPC-L, so a buffer overrun is impossible. But since
it is so easy to allow a buffer overrun, enforce the length checks for
everyone.
FIXES 135043
Change-Id: Iaf913e2156af2081c72125ec066d9636086c7d14
Reviewed-on: https://gerrit.openafs.org/15920
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
CVE-2024-10397
Currently, a few RPCs with arrays or opaque OUT arguments are called
with preallocated memory for the arg, but also provide a _len of 0 (or
an uninitialized _len). This makes it impossible for the xdr routine to
tell whether we have allocated enough space to actually hold the
response from the server.
To help this situation, either specify an appropriate _len for the
preallocated value (cm_IoctlGetACL, fsprobe_LWP), or don't provide a
preallocated buffer at all and let xdr allocate a buffer for us
(PGetAcl).
Note that this commit doesn't change xdr to actually check the value of
the given _len; but now a future commit can do so without breaking
callers.
FIXES 135043
Change-Id: Ieb50aaa5ae9a1bde027999ce1c668e0c99b4d82b
Reviewed-on: https://gerrit.openafs.org/15919
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
CVE-2024-10397
Currently, several callers call RPCs with string OUT arguments, and
provide preallocated memory for those arguments. This can easily allow a
response from the server to overrun the allocated buffer, stomping over
stack or heap memory.
We could simply make our preallocated buffers larger than the maximum
size that the RPC allows, but relying on that is error prone, and
there's no way for XDR to check if a string buffer is large enough.
Instead, to make sure we don't overrun a given preallocated buffer,
avoid giving a preallocated buffer to such RPCs, and let XDR allocate
the memory for us.
Specifically, this commit changes several callers to
RXAFS_GetVolumeStatus(), and one caller of BOZO_GetInstanceParm(), to
avoid passing in a preallocated string buffer.
All other callers of RPCs with string OUT args already let XDR allocate
the buffers for them.
FIXES 135043
Change-Id: If42e2cc983903cff9766e1bab487142d4d493a17
Reviewed-on: https://gerrit.openafs.org/15918
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
CVE-2024-10396
Several places in the tree parse ACLs using sscanf() calls that look
similar to this:
sscanf(str, "%d dfs:%d %s", &nplus, &dfs, cell);
sscanf(str, "%100s %d", tname, &trights);
Some callers check whether the scanf() returns negative or 0, but some
callers do not check the return code at all. If only some of the fields
are present in the sscanf()'d string (because, for instance, the ACL is
malformed), some of the arguments are left alone, and may be set to
garbage if the relevant variable was never initialized.
If the parsed ACL is copied to another ACL, this can result in the
copied ACL containing uninitialized memory.
To avoid this, make sure all of the variables passed to sscanf() and
similar calls are initialized before parsing. This commit does not
guarantee that the results make sense, but at least the results do not
contain uninitialized memory.
Change-Id: Ib0becffbce5a6e15f91ac4390bb9c422d478bea6
Reviewed-on: https://gerrit.openafs.org/15917
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
CVE-2024-10396
After the preceding commits, the data returned by the VIOCGETAL
pioctl (a RXAFS_FetchAcl wrapper) will safely be NUL-terminated.
However, the callers that attempt to parse the ACL string make
assumptions that the returned data will be properly formatted,
and implement a "skip to next line" functionality (under various
names) that blindly increments a char* until it finds a newline
character, which can read past the end of even a properly
NUL-terminated string if there is not a newline where one is
expected.
Adjust the various "skip to next line" functionality to keep
the current string pointer at the trailing NUL if the end of the
string is reached while searching for a newline.
Change-Id: I7fb7f23d7d6f68608f3e656a1530a7fc40b4a567
Reviewed-on: https://gerrit.openafs.org/15916
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
CVE-2024-10396
Supplement the previous commit by additionally verifying that
the returned ACL string occupies the entire XDR opaque, rejecting
any values returned that have an internal NUL prior to the end
of the opaque.
Change-Id: Iefa3d00a9a0e25ef66b7166fe952aae0603ee3d7
Reviewed-on: https://gerrit.openafs.org/15915
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
