OPENAFS-SA-2019-001, OPENAFS-SA-2019-002, and OPENAFS-SA-2019-003.
Add the signed advisory texts, patches, and appropriate notes
on the main security page.
Also fix the table entry for OPENAFS-SA-2018-003 at the bottom of the
page.
Change-Id: I006ded6b437c4820b5171c83f0f396d32f6f9349
Reviewed-on: https://gerrit.openafs.org/13925
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Tested-by: Benjamin Kaduk <kaduk@mit.edu>
--- /dev/null
+-----BEGIN PGP SIGNED MESSAGE-----
+Hash: SHA512
+
+OpenAFS Security Advisory 2019-001
+
+Topic: information leakage from uninitialized RPC output variables on error
+
+Issued: 22 October, 2019
+Affected: OpenAFS versions 1.0 through 1.6.23, and 1.8.0 through 1.8.4
+
+Generated RPC handler routines ran output variables through XDR encoding
+even when the call had failed and would shortly be aborted (and for
+which uninitialized output variables is common); any complete packets
+assembled in the process would be sent to the peer, leaking the contents
+of the uninitialized memory in question.
+
+SUMMARY
+=======
+
+AFS-3 RPCs use the XDR data encoding to represent input/output arguments,
+and OpenAFS uses an XDR en/decoder that is tightly integrated to the Rx
+protocol implementation. The generated RPC handler code takes care of
+allocating storage/variables for the input/output variables and reading the
+input parameters from the network into local storage before calling the
+function that implements the RPC logic. Similarly, after that function
+call, the generated code takes care of encoding the output parameters into
+Rx packets and sending them to the peer as necessary, then deallocating the
+storage it allocated and returning control flow to the caller. However, in
+the case of an error return from the handler, the Rx layer will abort the
+current Rx call to direct the peer that the operation failed, and
+well-behaved peers will discard any information obtained as part of the
+aborted call. Unfortunately, the generated code was going through the
+"encode output parameters" step even when the resulting data would not be of
+use to well-behaved peers, and in some cases the output parameters remained
+uninitialized in the case of error returns (on the assumption that since it
+was an error, the call would be aborted). So for non-well-behaved peers, a
+large partially constructed response could include information from
+uninitialized memory in the RPC handler.
+
+IMPACT
+======
+
+RPCs that produce large responses are most at risk, and configurations that
+use small MTU values, since those increate the likelihood of one or more
+complete Rx packets being filled. Leaving output variables uninitialized on
+error returns is a fairly common phenomenon, so many RPCs are expected to be
+at risk. The amount and rate of data exposure is highly context-dependent,
+since the memory allocator behavior and other load on the system affect
+exactly what information is leaked from uninitialized memory.
+
+At present, no code paths in the cache manager are known to trigger this
+issue, but the risk has not been conclusively shown to be absent, so clients
+are listed as potentially affected as well.
+
+AFFECTED SOFTWARE
+=================
+
+All releases of OpenAFS prior to 1.6.24 are affected, as are OpenAFS
+1.8.0 through 1.8.4.
+
+FIXES
+=====
+
+The OpenAFS project recommends that administrators upgrade all machines
+to the 1.8.5 or 1.6.24 releases. It is necessary to restart the server
+processes and cache manager in order for the fixes to take effect.
+
+DETAILS
+=======
+
+The AFS-3 protocol suite and OpenAFS extensions are run over the Rx RPC
+protocol. A variant of Sun rpcgen, rxgen, is used to generate data
+en/decoding routines and client- and server-side stub functions from an
+interface description file. In the RPC server, these stubs interface
+between the Rx network protocol layer and the implementation of the RPC
+server-side logic. The server logic is written as a normal C function,
+taking input variables either as pointer-to-struct or scalar-by-value, and
+returning output variables via pointer arguments (allocating storage as
+appropriate). The generated stub allocates storage on the stack for local
+variables to hold the input and ouput arguments of the RPC that are passed
+as arguments to the function implementing the server logic, and as discussed
+in OPENAFS-SA-2019-002, these local variables are only sometimes given an
+initial value; at other times they remain uninitialized and potentially
+refer to other content in the memory address space of the running process.
+
+Once the function implementing the server logic returns to the generated
+stub, the stub function uses the XDR encoder to pack the output variables
+from the local variables into an XDR bytestream. The way in which the
+OpenAFS encoder works results in Rx packets being generated by the XDR
+encoder, and when a complete packet is assembled, it is sent to the peer
+according to the Rx congestion-control logic. After all the output
+parameters have been encoded to the XDR bytestream, the generated stub frees
+any storage allocated, and then returns the result from the server logic.
+On success, this translates to completing any partially constructed Rx
+packet and sending the response code, but for error returns, any pending
+data is discarded and instead an Rx abort packet is sent for the Rx call
+over which the RPC is running. The abort indicates to the client that the
+RPC has failed and the XDR stream is to be discarded.
+
+Unfortunately, the XDR-encode-and-send-packets step is not skipped in the
+case of error returns, even though the call will ultimately be aborted and
+well-behaved clients will discard the XDR bytestream, there is still an
+opportunity for Rx packets to be generated and sent. When combined with the
+potential for uninitialized local variables to be included in the encoded
+stream, this results in a channel for the contents of uninitialized memory
+to be sent on the wire to a (potentially unauthenticated) peer.
+
+Vulnerabilities of this nature can be difficult to exploit, as there is not
+always context for *which* content from uninitialized memory is leaked to
+the caller, and that is in turn dependent on the allocation/deallocation
+patterns in the surrounding application and the other load patterns on the
+server. However, there are worked examples in other software of attackers
+being able to massage allocator state and stack contents in order to
+exfiltrate specific information, so it should be assumed that such attacks
+are also possible here given a sufficiently dedicated attacker.
+
+ACKNOWLEDGMENTS
+===============
+
+Thanks to Andrew Deason for the detailed report and contributed patch.
+-----BEGIN PGP SIGNATURE-----
+
+iQG3BAEBCgAdFiEE2WGV4E2ARf9BYP0XKNmm82TrdRIFAl2vh/QACgkQKNmm82Tr
+dRJGMwwghX7qcmKA50FBq+Smhvqp1KggNnGXnt6t78opKm3uR3LfTrsDc4g4YZLG
+4H24JdbYoGfyoziTJtBMLW8KubFojrZfoqgDRXgcpx2TQE36X0kzgpH2grAxE5kA
+wBL6aic6FxEgkF9Xw+iwo34hGv79WPHEgB0lWlGqjRj94geVh7dWjYWeL0K7JnlS
+WdOpmIWMUr5aMIG5G34t/XrZvoVDOrBjBKXUXFHLpYCZS7XPrxWTCOzNwyS5WehQ
+uBtEIgPeYVL1c+8XLA94Lyyegc8lnvdNvEU3nP9nIQTJ4x79j3bQ/PhmKQCQPMeS
+3G/LKtzYTA5d2SmEQ5pfTUYCmG4ULjAdk2IT2nW20cwbWmuArzWlcCPkAH3itonO
+ToRpRcywjqa1Z9p0EWrwZyXKVwRt1t8areIraHiFQiuTpjecl4or4ioXggnNoeqa
+Ek+W/sV5sTVFZhLUlpx2VrpvahqUckc4deo/BjUzqMpPWSLZ9tgH/Qv4pCkxtD29
+lvMgEDkke90EOQ==
+=KqJn
+-----END PGP SIGNATURE-----
--- /dev/null
+-----BEGIN PGP SIGNED MESSAGE-----
+Hash: SHA512
+
+OpenAFS Security Advisory 2019-002
+
+Topic: information leakage from uninitialized scalars
+
+Issued: 22 October, 2019
+Affected: OpenAFS versions 1.0 through 1.6.23, and 1.8.0 through 1.8.4
+
+Generated RPC handler routines only initialized local variables that would
+need to be freed, leaving uninitialized memory contents in RPC
+parameters of scalar type. When combined with RPC implementations that
+also did not supply output values, this results in the contents of
+uninitialized memory being sent to the peer, even for successful RPCs.
+
+SUMMARY
+=======
+
+AFS-3 RPCs use the XDR data encoding to represent input/output arguments,
+and OpenAFS uses an XDR en/decoder that is tightly integrated to the Rx
+protocol implementation. The generated RPC handler code takes care of
+allocating storage/variables for the input/output variables and reading the
+input parameters from the network into local storage before calling the
+function that implements the RPC logic. Similarly, after that function
+call, the generated code takes care of encoding the output parameters into
+Rx packets and sending them to the peer as necessary, then deallocating the
+storage it allocated and returning control flow to the caller. While
+the generated code is careful to initialize the local variables that
+might contain pointers to allocated memory (to ensure leak-free
+operation), scalar values (e.g., integers, flat structures) were not
+initialized. Some RPC handlers in OpenAFS do not always write to all output
+variables, even in the case of a successful return, so the contents of these
+uninitialized variables would be encoded into the XDR stream and sent to the
+(potentially unauthenticated) peer.
+
+IMPACT
+======
+
+It is uncommon for the server-side implementation of an RPC to not write
+to its output variables on the success case, though multiple occurrences
+have been found during an incomplete survey, leaking between 4 and 8 bytes
+per call. However, the actual amount and rate of data exposure is highly
+context-dependent, since the memory allocator behavior and other load on the
+system affect exactly what information is leaked from uninitialized memory.
+
+AFFECTED SOFTWARE
+=================
+
+All releases of OpenAFS prior to 1.6.24 are affected, as are OpenAFS
+1.8.0 through 1.8.4.
+
+FIXES
+=====
+
+The OpenAFS project recommends that administrators upgrade all machines
+to the 1.8.5 or 1.6.24 releases. It is necessary to restart the server
+processes and cache manager in order for the fixes to take effect.
+
+DETAILS
+=======
+
+The AFS-3 protocol suite and OpenAFS extensions are run over the Rx RPC
+protocol. A variant of Sun rpcgen, rxgen, is used to generate data
+en/decoding routines and client- and server-side stub functions from an
+interface description file. In the RPC server, these stubs interface
+between the Rx network protocol layer and the implementation of the RPC
+server-side logic. The server logic is written as a normal C function,
+taking input variables either as pointer-to-struct or scalar-by-value, and
+returning output variables via pointer arguments (allocating storage as
+appropriate). The generated stub allocates storage on the stack for local
+variables to hold the input and ouput arguments of the RPC that are passed
+as arguments to the function implementing the server logic, but (as is
+common in code of this vintage) only initializes some of the variables
+to known values. In particular, only the variables that will need to be
+passed to memory deallocation routines at the end of the function are
+initialized, to ensure that a garbage pointer is not passed to free().
+That leaves scalar types (integers, flat structures, etc.) with no
+explicit initialization, so references to them retrieve whatever
+happened to previously occupy that location in the program's address
+space.
+
+If an RPC implementation does not write to all its output variables (for
+example, if they correspond to some functionality that was not requested
+by the client or is disabled by configuration), then program execution
+will reach the end of the generated stub function without any value
+having been explicitly assigned to these (scalar) variables. In
+particular, these uninitialized values are passed to the XDR encoder
+routine and (as is common in code of this vintage) only initializes
+some of the variables to known values. In particular, only the
+variables that will need to be passed to memory deallocation routines at
+the end of the function are initialized, to ensure that a garbage
+pointer is not passed to free(). That leaves scalar types (integers,
+flat structures, etc.) with no explicit initialization, so references to
+them retrieve whatever happened to previously occupy that location in
+the program's address space. This includes when these variables are
+passed to the XDR encoder, so the uninitialized memory contents are
+encoded into the Rx stream and sent to the peer.
+
+Investigations surrounding this issue uncovered multiple RPCs known to leave
+output parameters uninitialized while returning success. However, since the
+risk is systemic and best addressed with a systemic fix, no effort was made
+to perform a comprehensive search and enumerate the RPCs with the flaw.
+
+As with most avenues for information disclosure from uninitialized
+memory, vulnerabilities of this nature can be difficult to exploit, as
+there is not always context for *which* content from uninitialized
+memory is leaked to the caller, and that is in turn dependent on the
+allocation/deallocation patterns in the surrounding application and the
+other load patterns on the server. However, there are worked examples
+in other software of attackers being able to massage allocator state and
+stack contents in order to exfiltrate specific information, so it should
+be assumed that such attacks are also possible here given a sufficiently
+dedicated attacker.
+
+ACKNOWLEDGMENTS
+===============
+
+Thanks to Andrew Deason for the detailed report and contributed patch.
+-----BEGIN PGP SIGNATURE-----
+
+iQG3BAEBCgAdFiEE2WGV4E2ARf9BYP0XKNmm82TrdRIFAl2vh/oACgkQKNmm82Tr
+dRLFwgweKkZFwtUAtZgpp8o96/yBTZ8iyuvDPYMN8LPobc/OqE6fxIdk8lwEQ696
+5F+N9nVXhwwVMqxP4PGw1qMVOyZFGwf3IXMMhgcZOQSepu6lwUeypRR/Ow0iFlI0
+XmeqI4F0PZGISpZlEc8HQ3/oASYWtMIatEs8vEWWpkrQLdYxKEZF2r9yMCdUwNwa
+QuqlEuORLa3aV4K7/c8JfWA3yi4GtYyoonZTeceS9s8MSHWgYtyH/0k3YQYsq57T
+vhJ+6MWG7WyNbbadJxngYACknaGtrk6Ifw2aIuKK1sUYKCqKvyO+BJXL68cBSupv
+rMG2+4RbKzmQXikQd3MxINTtu8ADhyqwQJi/UYHQXcHgyQJuIDKVu0W0PBJuKLQY
+u1JYB86ewsxOWazKIK1jbcMf15W6Jxhg6e1Wt5KrEFdoOinDgCnR7qWfPMxdLOQK
+UNxRX3T7wKZOFokU18dMX+TCKebb9fzxE0An/U5RsjtXWTodfi1Zo3A76KO1wFOn
+GCk+wKC+4tFeZg==
+=3pYF
+-----END PGP SIGNATURE-----
--- /dev/null
+-----BEGIN PGP SIGNED MESSAGE-----
+Hash: SHA512
+
+OpenAFS Security Advisory 2019-003
+
+Topic: database server crash from unserialized data access
+
+Issued: 22 October, 2019
+Affected: OpenAFS versions 1.0 through 1.6.23, and 1.8.0 through 1.8.4
+
+Database servers in the SVOTE_Debug() RPC handler can encounter a
+segmentation fault and crash during certain circumstances, when running in
+parallel with the end of a write transaction on the database.
+
+SUMMARY
+=======
+
+The SVOTE_Debug() RPC handler used to implement the main udebug(1)
+functionality performs unlocked accesses to a global pointer variable,
+dereferencing it if the first access returns a non-NULL value. Since the
+accesses are not serialized against writes, the value can change between
+accesses (e.g., to become NULL), with the attempted dereference generating a
+segmentation fault.
+
+IMPACT
+======
+
+The impact of this issue can be limited by the compiler and options used to
+compile the server code in question. For example, gcc on some popular linux
+distributions is able to optimize the two memory accesses into a single
+load, so the value being dereferenced to check the type of the transaction
+will never be NULL. (It might point into freed memory, but for the memory
+allocators in use on these linux systems, that memory is highly unlikely to
+have been unmapped in the intervening period, so the access would merely
+return bad data rather than crashing.) Other compilers/architectures do not
+make this optimization, such as the Solaris cc, and the resulting binaries
+are impacted.
+
+Since the VOTE_Debug() RPC does not require authentication, a remote
+attacker can attempt to call the RPC repeatedly and overlap with the end of
+a write transaction; crashes have been observed simply by running udebug(1)
+in a tight loop.
+
+AFFECTED SOFTWARE
+=================
+
+All releases of OpenAFS prior to 1.6.24 are affected, as are OpenAFS
+1.8.0 through 1.8.4.
+
+Database server processes built against LWP (instead of pthreads) are
+not affected by this issue. Database server processes are built
+against LWP by default in 1.6 releases, but are built against pthreads by
+default in 1.8 releases.
+
+FIXES
+=====
+
+The OpenAFS project recommends that administrators upgrade all database servers
+to the 1.8.5 or 1.6.24 releases. It is necessary to restart the database server
+processes in order for the fixes to take effect.
+
+Using a firewall to block or rate-limit VOTE_Debug() RPCs from untrusted
+sources may be an adequate workaround.
+
+DETAILS
+=======
+
+The udebug(1) utility is a debugging tool, and as such does not require
+authenticated access for the RPC calls it performs. Most of its
+functionality comes from the VOTE_Debug() RPC, which was written to
+prioritize speed and avoiding disruption of normal database operation over
+correctness of output. Accordingly, it accesses global state without
+adhering to the normal locking/serialization mechanisms for access to that
+state. This means that the returned results can be incorrect or
+inconsistent, but that tradeoff was part of its original design.
+
+One of the pieces of information returned by SVOTE_Debug() is whether there
+is a current transaction on the database, and if so, whether it is a write
+transaction. The current transaction is stored as a global pointer to a
+transaction structure (ubik_currentTrans), so whether or not there is an
+ongoing transaction is a question of whether ubik_currentTrans is non-NULL;
+the type of transaction (read vs. write) is stored in a field in the
+pointed-to structure. However, we only ever create a transaction structure
+for ubik_currentTrans in the case of a write transaction, so checking the
+type stored in the transaction structure provides no value for this case!
+
+Because SVOTE_Debug() does not take any locks, it can run in parallel with a
+running transaction, and in particular, the end of that transaction. This
+means that the value of ubik_currentTrans can change between accesses, and
+thus that the state determined by the first access ("there is a current
+transaction") can be invalid by the time of the second access (which
+dereferences the pointer). In this case, an invalid pointer is
+dereferenced, leading to a segmentation fault and application crash.
+
+ACKNOWLEDGMENTS
+===============
+
+Thanks to Andrew Deason for the detailed report and contributed patch.
+-----BEGIN PGP SIGNATURE-----
+
+iQG3BAEBCgAdFiEE2WGV4E2ARf9BYP0XKNmm82TrdRIFAl2vjWsACgkQKNmm82Tr
+dRLTfgwg1/96x8b3WpwMg4YUwE/RmFJZpKiI7/lscPcLsH3Jn4Ru72jqN5vUa9dW
+6nCYLNqOeSPm0QDqQOz2bzle1HlcTYaCOoh446Y2kwGpBWicpyAm70SRkEiPgnVr
+UR0XmfqIw0s5h7S98VPJYWeyOE6eQiH2pdjHsyg5duwg09e/Rvw+XBR4L4VQPlvx
+JpK1Aa5V+1//n0egOhImFxFCMh6a9PBkuHsxjn9++f3TmM744qUbFM2SsW20OtTo
+IaC518yt9Eoeyw84MnKdTzRVa6gNFcRq7wp8ls9t8zY46Q3aDEAnipkOo+2OVy7e
+2cLAZ4t6vvRJKH1FBbA58DsNM+wDoiKJO0+3xYquRmIs0EbkOVpE99XNu7FUX7w2
+c0xCobqlozhbuJIluSV/XbyETvKvQHg7MtKPcDN8Hgf7e12UxgisfcD8uOjb1Toj
+bpx+Y6VX3Du/tW82lbZ5/1jXjiluDtAwDSHXLQbGO2JzAdtuSqiQNDpITXSUTwiH
+rzYCDiJT9PivIg==
+=s4yw
+-----END PGP SIGNATURE-----
with PGP.
<hr>
+<a name="OPENAFS-SA-2019-003"></a>
+<h2>OPENAFS-SA-2019-003 - crash in database servers</h2>
+<table border=0>
+<tr><th align="left">Issued:</th><td>22-Oct-2019</td></tr>
+<tr><th align="left">Last Update:</th><td>22-Oct-2019</td></tr>
+<tr><th align="left">Severity:</th><td>Medium</td></tr>
+<tr><th align="left">Affected:</th><td>
+ OpenAFS server versions 1.0 through 1.6.23, 1.8.0 through 1.8.4 .</td></tr>
+<tr><th align="left">Patch:</th><td>
+ <a href="openafs-sa-2019-003-stable16.patch">
+ https://www.openafs.org/security/openafs-sa-2019-003-stable16.patch</a><br>
+</td><td>
+ <a href="openafs-sa-2019-003-stable18.patch">
+ https://www.openafs.org/security/openafs-sa-2019-003-stable18.patch</a><br>
+</td><td>
+ <a href="openafs-sa-2019-003-master.patch">
+ https://www.openafs.org/security/openafs-sa-2019-003-master.patch</a><br>
+</td></tr>
+<tr><th align="left">Full Text:</th><td>
+ <a href="OPENAFS-SA-2019-003.txt">
+ https://www.openafs.org/security/OPENAFS-SA-2019-003.txt</a>
+</td></tr>
+</table>
+<p>The ubik debugging RPCs prioritize being fast and non-disruptive to
+database operations over strict correctness, and do not adhere to the
+usual locking protocol for data access. A data race could cause a NULL
+dereference if the second memory load was not optimized out by the
+compiler
+
+<a name="OPENAFS-SA-2019-002"></a>
+<h2>OPENAFS-SA-2019-002 - information leakage from uninitialized scalars</h2>
+<table border=0>
+<tr><th align="left">Issued:</th><td>22-Oct-2019</td></tr>
+<tr><th align="left">Last Update:</th><td>22-Oct-2019</td></tr>
+<tr><th align="left">Severity:</th><td>Low</td></tr>
+<tr><th align="left">Affected:</th><td>
+ OpenAFS client versions 1.0 through 1.6.23, 1.8.0 through 1.8.4 ;
+ OpenAFS server versions 1.0 through 1.6.23, 1.8.0 through 1.8.4 .</td></tr>
+<tr><th align="left">Patch:</th><td>
+ <a href="openafs-sa-2019-002-stable16.patch">
+ https://www.openafs.org/security/openafs-sa-2019-002-stable16.patch</a><br>
+</td><td>
+ <a href="openafs-sa-2019-002-stable18.patch">
+ https://www.openafs.org/security/openafs-sa-2019-002-stable18.patch</a><br>
+</td><td>
+ <a href="openafs-sa-2019-002-master.patch">
+ https://www.openafs.org/security/openafs-sa-2019-002-master.patch</a><br>
+</td></tr>
+<tr><th align="left">Full Text:</th><td>
+ <a href="OPENAFS-SA-2019-002.txt">
+ https://www.openafs.org/security/OPENAFS-SA-2019-002.txt</a>
+</td></tr>
+</table>
+<p>Generated RPC handler routines did not initialize output variables of
+scalar (fixed-length) type, since they did not require dedicated logic to
+free. Such variables allocated on the stack could remain uninitialized
+in some cases (including those affected by OPENAFS-SA-2019-001), and the
+contents of uninitialized memory would be returned to the peer.
+
+<a name="OPENAFS-SA-2019-001"></a>
+<h2>OPENAFS-SA-2019-001 - information leakage in failed RPC output</h2>
+<table border=0>
+<tr><th align="left">Issued:</th><td>22-Oct-2019</td></tr>
+<tr><th align="left">Last Update:</th><td>22-Oct-2019</td></tr>
+<tr><th align="left">Severity:</th><td>Low</td></tr>
+<tr><th align="left">Affected:</th><td>
+ OpenAFS client versions 1.0 through 1.6.23, 1.8.0 through 1.8.4 ;
+ OpenAFS server versions 1.0 through 1.6.23, 1.8.0 through 1.8.4 .</td></tr>
+<tr><th align="left">Patch:</th><td>
+ <a href="openafs-sa-2019-001-stable16.patch">
+ https://www.openafs.org/security/openafs-sa-2019-001-stable16.patch</a><br>
+</td><td>
+ <a href="openafs-sa-2019-001-stable18.patch">
+ https://www.openafs.org/security/openafs-sa-2019-001-stable18.patch</a><br>
+</td><td>
+ <a href="openafs-sa-2019-001-master.patch">
+ https://www.openafs.org/security/openafs-sa-2019-001-master.patch</a><br>
+</td></tr>
+<tr><th align="left">Full Text:</th><td>
+ <a href="OPENAFS-SA-2019-001.txt">
+ https://www.openafs.org/security/OPENAFS-SA-2019-001.txt</a>
+</td></tr>
+</table>
+<p>Generated RPC handler routines ran output variables through XDR encoding
+even when the call had failed and would shortly be aborted (and for
+which uninitialized output variables is common); any complete packets
+assembled in the process would be sent to the peer, leaking the contents
+of the uninitialized memory in question.
+
<a name="OPENAFS-SA-2018-003"></a>
<h2>OPENAFS-SA-2018-003 - denial of service due to excess resource consumption</h2>
<table border=0>
<td><a href="#OPENAFS-SA-2018-002">information leakage from uninitialized RPC output variables</a></td>
</tr>
<tr>
- <td>2018-002</td><td>11-Sep-2018</td><td>11-Sep-2018</td>
+ <td>2018-003</td><td>11-Sep-2018</td><td>11-Sep-2018</td>
<td>Medium</td><td>1.0-1.6.22.4,1.8.0-1.8.1.1</td>
<td><a href="#OPENAFS-SA-2018-003">denial of service due to excess resource consumption</a></td>
</tr>
+<tr>
+ <td>2019-001</td><td>22-Oct-2019</td><td>22-Oct-2019</td>
+ <td>Low</td><td>1.0-1.6.23,1.8.0-1.8.4</td>
+ <td><a href="#OPENAFS-SA-2019-001">information leakage in failed RPC output</a></td>
+</tr>
+<tr>
+ <td>2019-002</td><td>22-Oct-2019</td><td>22-Oct-2019</td>
+ <td>Low</td><td>1.0-1.6.23,1.8.0-1.8.4</td>
+ <td><a href="#OPENAFS-SA-2019-002">information leakage from uninitialized scalars</a></td>
+</tr>
+<tr>
+ <td>2019-003</td><td>22-Oct-2019</td><td>22-Oct-2019</td>
+ <td>Medium</td><td>1.0-1.6.23,1.8.0-1.8.4</td>
+ <td><a href="#OPENAFS-SA-2019-003">crash in database servers</a></td>
+</tr>
</table>
--- /dev/null
+commit ea276e83e37e5bd27285a3d639f2158639172786
+Author: Andrew Deason <adeason@sinenomine.net>
+Date: Wed Aug 7 20:50:47 2019 -0500
+
+ OPENAFS-SA-2019-001: Skip server OUT args on error
+
+ Currently, part of our server-side RPC argument-handling code that's
+ generated from rxgen looks like this (for example):
+
+ z_result = SRXAFS_BulkStatus(z_call, &FidsArray, &StatArray, &CBArray, &Sync);
+ z_xdrs->x_op = XDR_ENCODE;
+ if ((!xdr_AFSBulkStats(z_xdrs, &StatArray))
+ || (!xdr_AFSCBs(z_xdrs, &CBArray))
+ || (!xdr_AFSVolSync(z_xdrs, &Sync)))
+ z_result = RXGEN_SS_MARSHAL;
+ fail:
+ [...]
+ return z_result;
+
+ When the server routine for implementing the RPC results a non-zero
+ value into z_result, the call will be aborted. However, before we
+ abort the call, we still call the xdr_* routines with XDR_ENCODE for
+ all of our output arguments. If the call has not already been aborted
+ for other reasons, we'll serialize the output argument data into the
+ Rx call. If we push more data than can fit in a single Rx packet for
+ the call, then we'll also send that data to the client. Many server
+ routines for implementing RPCs do not initialize the memory inside
+ their output arguments during certain errors, and so the memory may be
+ leaked to the peer.
+
+ To avoid this, just jump to the 'fail' label when a nonzero 'z_result'
+ is returned. This means we skip sending the output argument data to
+ the peer, but we still free any argument data that needs freeing, and
+ record the stats for the call (if needed). This makes the above
+ example now look like this:
+
+ z_result = SRXAFS_BulkStatus(z_call, &FidsArray, &StatArray, &CBArray, &Sync);
+ if (z_result)
+ goto fail;
+ z_xdrs->x_op = XDR_ENCODE;
+ if ((!xdr_AFSBulkStats(z_xdrs, &StatArray))
+ || (!xdr_AFSCBs(z_xdrs, &CBArray))
+ || (!xdr_AFSVolSync(z_xdrs, &Sync)))
+ z_result = RXGEN_SS_MARSHAL;
+ fail:
+ [...]
+ return z_result;
+
+ Change-Id: I2bdea2e808bb215720492b0ba6ac1a88da61b954
+ Reviewed-on: https://gerrit.openafs.org/13913
+ Reviewed-by: Andrew Deason <adeason@sinenomine.net>
+ Tested-by: BuildBot <buildbot@rampaginggeek.com>
+ Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
+
+diff --git a/src/rxgen/rpc_parse.c b/src/rxgen/rpc_parse.c
+index 492bfb0853..4e1b6141d7 100644
+--- a/src/rxgen/rpc_parse.c
++++ b/src/rxgen/rpc_parse.c
+@@ -1301,7 +1301,12 @@ cs_ProcTail_setup(definition * defp, int split_flag)
+ static void
+ ss_Proc_CodeGeneration(definition * defp)
+ {
+- defp->can_fail = 0;
++ extern char zflag;
++
++ if (zflag)
++ defp->can_fail = 0;
++ else
++ defp->can_fail = 1;
+ ss_ProcName_setup(defp);
+ if (!cflag) {
+ ss_ProcParams_setup(defp);
+@@ -1546,6 +1551,8 @@ ss_ProcCallRealProc_setup(definition * defp)
+ f_print(fout, ");\n");
+ if (zflag) {
+ f_print(fout, "\tif (z_result)\n\t\treturn z_result;\n");
++ } else {
++ f_print(fout, "\tif (z_result)\n\t\tgoto fail;\n");
+ }
+ }
+
--- /dev/null
+commit 6e08c0368aa43fa9851556050f2e4bf4e59edaea
+Author: Andrew Deason <adeason@sinenomine.net>
+Date: Wed Aug 7 20:50:47 2019 -0500
+
+ OPENAFS-SA-2019-001: Skip server OUT args on error
+
+ Currently, part of our server-side RPC argument-handling code that's
+ generated from rxgen looks like this (for example):
+
+ z_result = SRXAFS_BulkStatus(z_call, &FidsArray, &StatArray, &CBArray, &Sync);
+ z_xdrs->x_op = XDR_ENCODE;
+ if ((!xdr_AFSBulkStats(z_xdrs, &StatArray))
+ || (!xdr_AFSCBs(z_xdrs, &CBArray))
+ || (!xdr_AFSVolSync(z_xdrs, &Sync)))
+ z_result = RXGEN_SS_MARSHAL;
+ fail:
+ [...]
+ return z_result;
+
+ When the server routine for implementing the RPC results a non-zero
+ value into z_result, the call will be aborted. However, before we
+ abort the call, we still call the xdr_* routines with XDR_ENCODE for
+ all of our output arguments. If the call has not already been aborted
+ for other reasons, we'll serialize the output argument data into the
+ Rx call. If we push more data than can fit in a single Rx packet for
+ the call, then we'll also send that data to the client. Many server
+ routines for implementing RPCs do not initialize the memory inside
+ their output arguments during certain errors, and so the memory may be
+ leaked to the peer.
+
+ To avoid this, just jump to the 'fail' label when a nonzero 'z_result'
+ is returned. This means we skip sending the output argument data to
+ the peer, but we still free any argument data that needs freeing, and
+ record the stats for the call (if needed). This makes the above
+ example now look like this:
+
+ z_result = SRXAFS_BulkStatus(z_call, &FidsArray, &StatArray, &CBArray, &Sync);
+ if (z_result)
+ goto fail;
+ z_xdrs->x_op = XDR_ENCODE;
+ if ((!xdr_AFSBulkStats(z_xdrs, &StatArray))
+ || (!xdr_AFSCBs(z_xdrs, &CBArray))
+ || (!xdr_AFSVolSync(z_xdrs, &Sync)))
+ z_result = RXGEN_SS_MARSHAL;
+ fail:
+ [...]
+ return z_result;
+
+ Reviewed-on: https://gerrit.openafs.org/13913
+ Reviewed-by: Andrew Deason <adeason@sinenomine.net>
+ Tested-by: BuildBot <buildbot@rampaginggeek.com>
+ Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
+ (cherry picked from commit ea276e83e37e5bd27285a3d639f2158639172786)
+
+ Reviewed-on: https://gerrit.openafs.org/13916
+ Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
+ Tested-by: Benjamin Kaduk <kaduk@mit.edu>
+ (cherry picked from commit 5a3d1b62810fc8cc7b37a737b4f5f1912bc614f9)
+
+ Change-Id: Ia54b06399b1f8a05a560ec7560580783d3e64b9d
+ Reviewed-on: https://gerrit.openafs.org/13921
+ Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
+ Tested-by: BuildBot <buildbot@rampaginggeek.com>
+
+diff --git a/src/rxgen/rpc_parse.c b/src/rxgen/rpc_parse.c
+index 5a3f27d845..8ff57d0794 100644
+--- a/src/rxgen/rpc_parse.c
++++ b/src/rxgen/rpc_parse.c
+@@ -1369,8 +1369,12 @@ static void
+ ss_Proc_CodeGeneration(definition * defp)
+ {
+ int somefrees = 0;
++ extern char zflag;
+
+- defp->can_fail = 0;
++ if (zflag)
++ defp->can_fail = 0;
++ else
++ defp->can_fail = 1;
+ ss_ProcName_setup(defp);
+ if (!cflag) {
+ ss_ProcParams_setup(defp, &somefrees);
+@@ -1610,6 +1614,8 @@ ss_ProcCallRealProc_setup(definition * defp)
+ f_print(fout, ");\n");
+ if (zflag) {
+ f_print(fout, "\tif (z_result)\n\t\treturn z_result;\n");
++ } else {
++ f_print(fout, "\tif (z_result)\n\t\tgoto fail;\n");
+ }
+ }
+
--- /dev/null
+commit 5a3d1b62810fc8cc7b37a737b4f5f1912bc614f9
+Author: Andrew Deason <adeason@sinenomine.net>
+Date: Wed Aug 7 20:50:47 2019 -0500
+
+ OPENAFS-SA-2019-001: Skip server OUT args on error
+
+ Currently, part of our server-side RPC argument-handling code that's
+ generated from rxgen looks like this (for example):
+
+ z_result = SRXAFS_BulkStatus(z_call, &FidsArray, &StatArray, &CBArray, &Sync);
+ z_xdrs->x_op = XDR_ENCODE;
+ if ((!xdr_AFSBulkStats(z_xdrs, &StatArray))
+ || (!xdr_AFSCBs(z_xdrs, &CBArray))
+ || (!xdr_AFSVolSync(z_xdrs, &Sync)))
+ z_result = RXGEN_SS_MARSHAL;
+ fail:
+ [...]
+ return z_result;
+
+ When the server routine for implementing the RPC results a non-zero
+ value into z_result, the call will be aborted. However, before we
+ abort the call, we still call the xdr_* routines with XDR_ENCODE for
+ all of our output arguments. If the call has not already been aborted
+ for other reasons, we'll serialize the output argument data into the
+ Rx call. If we push more data than can fit in a single Rx packet for
+ the call, then we'll also send that data to the client. Many server
+ routines for implementing RPCs do not initialize the memory inside
+ their output arguments during certain errors, and so the memory may be
+ leaked to the peer.
+
+ To avoid this, just jump to the 'fail' label when a nonzero 'z_result'
+ is returned. This means we skip sending the output argument data to
+ the peer, but we still free any argument data that needs freeing, and
+ record the stats for the call (if needed). This makes the above
+ example now look like this:
+
+ z_result = SRXAFS_BulkStatus(z_call, &FidsArray, &StatArray, &CBArray, &Sync);
+ if (z_result)
+ goto fail;
+ z_xdrs->x_op = XDR_ENCODE;
+ if ((!xdr_AFSBulkStats(z_xdrs, &StatArray))
+ || (!xdr_AFSCBs(z_xdrs, &CBArray))
+ || (!xdr_AFSVolSync(z_xdrs, &Sync)))
+ z_result = RXGEN_SS_MARSHAL;
+ fail:
+ [...]
+ return z_result;
+
+ Reviewed-on: https://gerrit.openafs.org/13913
+ Reviewed-by: Andrew Deason <adeason@sinenomine.net>
+ Tested-by: BuildBot <buildbot@rampaginggeek.com>
+ Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
+ (cherry picked from commit ea276e83e37e5bd27285a3d639f2158639172786)
+
+ Change-Id: I688cbf1a65903bf26a0db033687898f3fb5a54ea
+ Reviewed-on: https://gerrit.openafs.org/13916
+ Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
+ Tested-by: Benjamin Kaduk <kaduk@mit.edu>
+
+diff --git a/src/rxgen/rpc_parse.c b/src/rxgen/rpc_parse.c
+index f5d7c70338..032afe0313 100644
+--- a/src/rxgen/rpc_parse.c
++++ b/src/rxgen/rpc_parse.c
+@@ -1301,7 +1301,12 @@ cs_ProcTail_setup(definition * defp, int split_flag)
+ static void
+ ss_Proc_CodeGeneration(definition * defp)
+ {
+- defp->can_fail = 0;
++ extern char zflag;
++
++ if (zflag)
++ defp->can_fail = 0;
++ else
++ defp->can_fail = 1;
+ ss_ProcName_setup(defp);
+ if (!cflag) {
+ ss_ProcParams_setup(defp);
+@@ -1546,6 +1551,8 @@ ss_ProcCallRealProc_setup(definition * defp)
+ f_print(fout, ");\n");
+ if (zflag) {
+ f_print(fout, "\tif (z_result)\n\t\treturn z_result;\n");
++ } else {
++ f_print(fout, "\tif (z_result)\n\t\tgoto fail;\n");
+ }
+ }
+
--- /dev/null
+commit 93aee3cf40622993b95bd1af77080a31670c24bb
+Author: Andrew Deason <adeason@sinenomine.net>
+Date: Wed Aug 7 21:19:47 2019 -0500
+
+ OPENAFS-SA-2019-002: Zero all server RPC args
+
+ Currently, our server-side RPC argument-handling code generated from
+ rxgen initializes complex arguments like so (for example, in
+ _RXAFS_BulkStatus):
+
+ AFSCBFids FidsArray;
+ AFSBulkStats StatArray;
+ AFSCBs CBArray;
+ AFSVolSync Sync;
+
+ FidsArray.AFSCBFids_val = 0;
+ FidsArray.AFSCBFids_len = 0;
+ CBArray.AFSCBs_val = 0;
+ CBArray.AFSCBs_len = 0;
+ StatArray.AFSBulkStats_val = 0;
+ StatArray.AFSBulkStats_len = 0;
+
+ This is done for any input or output arguments, but only for types we
+ need to free afterwards (arrays, usually). We do not do this for
+ simple types, like single flat structs. In the above example, we do
+ this for the arrays FidsArray, StatArray, and CBArray, but 'Sync' is
+ not initialized to anything.
+
+ If some server RPC handlers never set a value for an output argument,
+ this means we'll send uninitialized stack memory to our peer.
+ Currently this can happen in, for example,
+ MRXSTATS_RetrieveProcessRPCStats if 'rxi_monitor_processStats' is
+ unset (specifically, the 'clock_sec' and 'clock_usec' arguments are
+ never set when rx_enableProcessRPCStats() has not been called).
+
+ To make sure we cannot send uninitialized data to our peer, change
+ rxgen to instead 'memset(&arg, 0, sizeof(arg));' for every single
+ parameter. Using memset in this way just makes this a little simpler
+ inside rxgen, since all we need to do this is the name of the
+ argument.
+
+ With this commit, the rxgen-generated code for the above example now
+ looks like this:
+
+ AFSCBFids FidsArray;
+ AFSBulkStats StatArray;
+ AFSCBs CBArray;
+ AFSVolSync Sync;
+
+ memset(&FidsArray, 0, sizeof(FidsArray));
+ memset(&CBArray, 0, sizeof(CBArray));
+ memset(&StatArray, 0, sizeof(StatsArray));
+ memset(&Sync, 0, sizeof(Sync));
+
+ Change-Id: Iedccc25e50ee32bd1144e652b951496cb7dde5d2
+ Reviewed-on: https://gerrit.openafs.org/13914
+ Reviewed-by: Andrew Deason <adeason@sinenomine.net>
+ Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
+ Tested-by: BuildBot <buildbot@rampaginggeek.com>
+
+diff --git a/src/rxgen/rpc_parse.c b/src/rxgen/rpc_parse.c
+index 4e1b6141d7..768e42f3ba 100644
+--- a/src/rxgen/rpc_parse.c
++++ b/src/rxgen/rpc_parse.c
+@@ -1435,8 +1435,6 @@ ss_ProcSpecial_setup(definition * defp)
+ if (streq(string, structname(plist->pl.param_type))) {
+ plist->pl.string_name = spec->sdef.string_name;
+ plist->pl.param_flag |= FREETHIS_PARAM;
+- fprintf(fout, "\n\t%s.%s = 0;", plist->pl.param_name,
+- spec->sdef.string_name);
+ }
+ }
+ }
+@@ -1451,22 +1449,13 @@ ss_ProcSpecial_setup(definition * defp)
+ case REL_ARRAY:
+ plist->pl.string_name = alloc(40);
+ if (brief_flag) {
+- f_print(fout, "\n\t%s.val = 0;",
+- plist->pl.param_name);
+- f_print(fout, "\n\t%s.len = 0;",
+- plist->pl.param_name);
+ s_print(plist->pl.string_name, "val");
+ } else {
+- f_print(fout, "\n\t%s.%s_val = 0;",
+- plist->pl.param_name, defp1->def_name);
+- f_print(fout, "\n\t%s.%s_len = 0;",
+- plist->pl.param_name, defp1->def_name);
+ s_print(plist->pl.string_name, "%s_val",
+ defp1->def_name);
+ }
+ break;
+ case REL_POINTER:
+- f_print(fout, "\n\t%s = 0;", plist->pl.param_name);
+ plist->pl.string_name = NULL;
+ break;
+ default:
+@@ -1482,13 +1471,19 @@ ss_ProcSpecial_setup(definition * defp)
+ if (plist->component_kind == DEF_PARAM) {
+ if (streq(defp1->def_name, structname(plist->pl.param_type))) {
+ plist->pl.param_flag |= FREETHIS_PARAM;
+- fprintf(fout, "\n\tmemset(&%s, 0, sizeof(%s));",
+- plist->pl.param_name, defp1->def_name);
+ }
+ }
+ }
+ }
+
++ for (plist = defp->pc.plists; plist; plist = plist->next) {
++ if (plist->component_kind == DEF_PARAM) {
++ fprintf(fout, "\n\tmemset(&%s, 0, sizeof(%s));",
++ plist->pl.param_name,
++ plist->pl.param_name);
++ }
++ }
++
+ f_print(fout, "\n");
+ }
+
--- /dev/null
+commit 2a7b4b891bec730a6c4f58e3b5976383e4c179c1
+Author: Andrew Deason <adeason@sinenomine.net>
+Date: Wed Aug 7 21:19:47 2019 -0500
+
+ OPENAFS-SA-2019-002: Zero all server RPC args
+
+ Currently, our server-side RPC argument-handling code generated from
+ rxgen initializes complex arguments like so (for example, in
+ _RXAFS_BulkStatus):
+
+ AFSCBFids FidsArray;
+ AFSBulkStats StatArray;
+ AFSCBs CBArray;
+ AFSVolSync Sync;
+
+ FidsArray.AFSCBFids_val = 0;
+ FidsArray.AFSCBFids_len = 0;
+ CBArray.AFSCBs_val = 0;
+ CBArray.AFSCBs_len = 0;
+ StatArray.AFSBulkStats_val = 0;
+ StatArray.AFSBulkStats_len = 0;
+
+ This is done for any input or output arguments, but only for types we
+ need to free afterwards (arrays, usually). We do not do this for
+ simple types, like single flat structs. In the above example, we do
+ this for the arrays FidsArray, StatArray, and CBArray, but 'Sync' is
+ not initialized to anything.
+
+ If some server RPC handlers never set a value for an output argument,
+ this means we'll send uninitialized stack memory to our peer.
+ Currently this can happen in, for example,
+ MRXSTATS_RetrieveProcessRPCStats if 'rxi_monitor_processStats' is
+ unset (specifically, the 'clock_sec' and 'clock_usec' arguments are
+ never set when rx_enableProcessRPCStats() has not been called).
+
+ To make sure we cannot send uninitialized data to our peer, change
+ rxgen to instead 'memset(&arg, 0, sizeof(arg));' for every single
+ parameter. Using memset in this way just makes this a little simpler
+ inside rxgen, since all we need to do this is the name of the
+ argument.
+
+ With this commit, the rxgen-generated code for the above example now
+ looks like this:
+
+ AFSCBFids FidsArray;
+ AFSBulkStats StatArray;
+ AFSCBs CBArray;
+ AFSVolSync Sync;
+
+ memset(&FidsArray, 0, sizeof(FidsArray));
+ memset(&CBArray, 0, sizeof(CBArray));
+ memset(&StatArray, 0, sizeof(StatsArray));
+ memset(&Sync, 0, sizeof(Sync));
+
+ Reviewed-on: https://gerrit.openafs.org/13914
+ Reviewed-by: Andrew Deason <adeason@sinenomine.net>
+ Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
+ Tested-by: BuildBot <buildbot@rampaginggeek.com>
+ (cherry picked from commit 93aee3cf40622993b95bd1af77080a31670c24bb)
+
+ Reviewed-on: https://gerrit.openafs.org/13917
+ Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
+ Tested-by: Benjamin Kaduk <kaduk@mit.edu>
+ (cherry picked from commit fcaac44f845d18d6fd5d2f3685db11118d8f8626)
+
+ Change-Id: Ic096570e9c894fb05d084ba451beabda3bb295e2
+ Reviewed-on: https://gerrit.openafs.org/13922
+ Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
+ Tested-by: BuildBot <buildbot@rampaginggeek.com>
+
+diff --git a/src/rxgen/rpc_parse.c b/src/rxgen/rpc_parse.c
+index 8ff57d0794..e3731dfc8a 100644
+--- a/src/rxgen/rpc_parse.c
++++ b/src/rxgen/rpc_parse.c
+@@ -1508,8 +1508,6 @@ ss_ProcSpecial_setup(definition * defp, int *somefrees)
+ plist->pl.string_name = spec->sdef.string_name;
+ plist->pl.param_flag |= FREETHIS_PARAM;
+ *somefrees = 1;
+- fprintf(fout, "\n\t%s.%s = 0;", plist->pl.param_name,
+- spec->sdef.string_name);
+ }
+ }
+ }
+@@ -1527,22 +1525,13 @@ ss_ProcSpecial_setup(definition * defp, int *somefrees)
+ case REL_ARRAY:
+ plist->pl.string_name = alloc(40);
+ if (brief_flag) {
+- f_print(fout, "\n\t%s.val = 0;",
+- plist->pl.param_name);
+- f_print(fout, "\n\t%s.len = 0;",
+- plist->pl.param_name);
+ s_print(plist->pl.string_name, "val");
+ } else {
+- f_print(fout, "\n\t%s.%s_val = 0;",
+- plist->pl.param_name, defp1->def_name);
+- f_print(fout, "\n\t%s.%s_len = 0;",
+- plist->pl.param_name, defp1->def_name);
+ s_print(plist->pl.string_name, "%s_val",
+ defp1->def_name);
+ }
+ break;
+ case REL_POINTER:
+- f_print(fout, "\n\t%s = 0;", plist->pl.param_name);
+ plist->pl.string_name = NULL;
+ break;
+ default:
+@@ -1552,6 +1541,15 @@ ss_ProcSpecial_setup(definition * defp, int *somefrees)
+ }
+ }
+ }
++
++ for (plist = defp->pc.plists; plist; plist = plist->next) {
++ if (plist->component_kind == DEF_PARAM) {
++ fprintf(fout, "\n\tmemset(&%s, 0, sizeof(%s));",
++ plist->pl.param_name,
++ plist->pl.param_name);
++ }
++ }
++
+ f_print(fout, "\n");
+ }
+
--- /dev/null
+commit fcaac44f845d18d6fd5d2f3685db11118d8f8626
+Author: Andrew Deason <adeason@sinenomine.net>
+Date: Wed Aug 7 21:19:47 2019 -0500
+
+ OPENAFS-SA-2019-002: Zero all server RPC args
+
+ Currently, our server-side RPC argument-handling code generated from
+ rxgen initializes complex arguments like so (for example, in
+ _RXAFS_BulkStatus):
+
+ AFSCBFids FidsArray;
+ AFSBulkStats StatArray;
+ AFSCBs CBArray;
+ AFSVolSync Sync;
+
+ FidsArray.AFSCBFids_val = 0;
+ FidsArray.AFSCBFids_len = 0;
+ CBArray.AFSCBs_val = 0;
+ CBArray.AFSCBs_len = 0;
+ StatArray.AFSBulkStats_val = 0;
+ StatArray.AFSBulkStats_len = 0;
+
+ This is done for any input or output arguments, but only for types we
+ need to free afterwards (arrays, usually). We do not do this for
+ simple types, like single flat structs. In the above example, we do
+ this for the arrays FidsArray, StatArray, and CBArray, but 'Sync' is
+ not initialized to anything.
+
+ If some server RPC handlers never set a value for an output argument,
+ this means we'll send uninitialized stack memory to our peer.
+ Currently this can happen in, for example,
+ MRXSTATS_RetrieveProcessRPCStats if 'rxi_monitor_processStats' is
+ unset (specifically, the 'clock_sec' and 'clock_usec' arguments are
+ never set when rx_enableProcessRPCStats() has not been called).
+
+ To make sure we cannot send uninitialized data to our peer, change
+ rxgen to instead 'memset(&arg, 0, sizeof(arg));' for every single
+ parameter. Using memset in this way just makes this a little simpler
+ inside rxgen, since all we need to do this is the name of the
+ argument.
+
+ With this commit, the rxgen-generated code for the above example now
+ looks like this:
+
+ AFSCBFids FidsArray;
+ AFSBulkStats StatArray;
+ AFSCBs CBArray;
+ AFSVolSync Sync;
+
+ memset(&FidsArray, 0, sizeof(FidsArray));
+ memset(&CBArray, 0, sizeof(CBArray));
+ memset(&StatArray, 0, sizeof(StatsArray));
+ memset(&Sync, 0, sizeof(Sync));
+
+ Reviewed-on: https://gerrit.openafs.org/13914
+ Reviewed-by: Andrew Deason <adeason@sinenomine.net>
+ Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
+ Tested-by: BuildBot <buildbot@rampaginggeek.com>
+ (cherry picked from commit 93aee3cf40622993b95bd1af77080a31670c24bb)
+
+ Change-Id: I6e19aaea57e545455b65851d1bedade584e482f0
+ Reviewed-on: https://gerrit.openafs.org/13917
+ Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
+ Tested-by: Benjamin Kaduk <kaduk@mit.edu>
+
+diff --git a/src/rxgen/rpc_parse.c b/src/rxgen/rpc_parse.c
+index 032afe0313..629e703504 100644
+--- a/src/rxgen/rpc_parse.c
++++ b/src/rxgen/rpc_parse.c
+@@ -1435,8 +1435,6 @@ ss_ProcSpecial_setup(definition * defp)
+ if (streq(string, structname(plist->pl.param_type))) {
+ plist->pl.string_name = spec->sdef.string_name;
+ plist->pl.param_flag |= FREETHIS_PARAM;
+- fprintf(fout, "\n\t%s.%s = 0;", plist->pl.param_name,
+- spec->sdef.string_name);
+ }
+ }
+ }
+@@ -1451,22 +1449,13 @@ ss_ProcSpecial_setup(definition * defp)
+ case REL_ARRAY:
+ plist->pl.string_name = alloc(40);
+ if (brief_flag) {
+- f_print(fout, "\n\t%s.val = 0;",
+- plist->pl.param_name);
+- f_print(fout, "\n\t%s.len = 0;",
+- plist->pl.param_name);
+ s_print(plist->pl.string_name, "val");
+ } else {
+- f_print(fout, "\n\t%s.%s_val = 0;",
+- plist->pl.param_name, defp1->def_name);
+- f_print(fout, "\n\t%s.%s_len = 0;",
+- plist->pl.param_name, defp1->def_name);
+ s_print(plist->pl.string_name, "%s_val",
+ defp1->def_name);
+ }
+ break;
+ case REL_POINTER:
+- f_print(fout, "\n\t%s = 0;", plist->pl.param_name);
+ plist->pl.string_name = NULL;
+ break;
+ default:
+@@ -1482,13 +1471,19 @@ ss_ProcSpecial_setup(definition * defp)
+ if (plist->component_kind == DEF_PARAM) {
+ if (streq(defp1->def_name, structname(plist->pl.param_type))) {
+ plist->pl.param_flag |= FREETHIS_PARAM;
+- fprintf(fout, "\n\tmemset(&%s, 0, sizeof(%s));",
+- plist->pl.param_name, defp1->def_name);
+ }
+ }
+ }
+ }
+
++ for (plist = defp->pc.plists; plist; plist = plist->next) {
++ if (plist->component_kind == DEF_PARAM) {
++ fprintf(fout, "\n\tmemset(&%s, 0, sizeof(%s));",
++ plist->pl.param_name,
++ plist->pl.param_name);
++ }
++ }
++
+ f_print(fout, "\n");
+ }
+
--- /dev/null
+commit 6ec46ba7773089e1549d27a0d345afeca65c9472
+Author: Andrew Deason <adeason@sinenomine.net>
+Date: Mon Sep 16 14:06:53 2019 -0500
+
+ OPENAFS-SA-2019-003: ubik: Avoid unlocked ubik_currentTrans deref
+
+ Currently, SVOTE_Debug/SVOTE_DebugOld examine some ubik internal state
+ without any locks, because the speed of these functions is more
+ important than accuracy.
+
+ However, one of the pieces of data we examine is ubik_currentTrans,
+ which we dereference to get ubik_currentTrans->type. ubik_currentTrans
+ could be set to NULL while this code is running, so there is a small
+ chance of this code causing a segfault, if SVOTE_Debug() is running
+ when the current transaction ends.
+
+ We only ever initialize ubik_currentTrans as a write transation (via
+ SDISK_Begin), so this check is pointless anyway. Accordingly, skip the
+ type check, and always assume that any active transaction is a write
+ transaction. This means we only ever access ubik_currentTrans once,
+ avoiding any risk of the value changing between accesses (and we no
+ longer need to dereference it, anyway).
+
+ Note that, since ubik_currentTrans is not marked as 'volatile', some C
+ compilers, with certain options, can and do assume that its value will
+ not change between accesses, and thus only fetch the pointer value once.
+ This avoids the risk of NULL dereference (and thus, crash, if pointer
+ stores/loads are atomic), but the value pointed to by ubik_currentTrans->type
+ would be incorrect when the transaction ends during the execution of
+ SVOTE_Debug().
+
+ Change-Id: Ia36c58e5906f5e8df59936f845ae11e886e8ec38
+ Reviewed-on: https://gerrit.openafs.org/13915
+ Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
+ Tested-by: Benjamin Kaduk <kaduk@mit.edu>
+
+diff --git a/src/ubik/vote.c b/src/ubik/vote.c
+index 34f285dcd2..1ae7456435 100644
+--- a/src/ubik/vote.c
++++ b/src/ubik/vote.c
+@@ -444,10 +444,7 @@ SVOTE_Debug(struct rx_call * rxcall, struct ubik_debug * aparm)
+
+ if (ubik_currentTrans) {
+ aparm->currentTrans = 1;
+- if (ubik_currentTrans->type == UBIK_WRITETRANS)
+- aparm->writeTrans = 1;
+- else
+- aparm->writeTrans = 0;
++ aparm->writeTrans = 1;
+ } else {
+ aparm->currentTrans = 0;
+ }
+@@ -527,10 +524,7 @@ SVOTE_DebugOld(struct rx_call * rxcall,
+
+ if (ubik_currentTrans) {
+ aparm->currentTrans = 1;
+- if (ubik_currentTrans->type == UBIK_WRITETRANS)
+- aparm->writeTrans = 1;
+- else
+- aparm->writeTrans = 0;
++ aparm->writeTrans = 1;
+ } else {
+ aparm->currentTrans = 0;
+ }
--- /dev/null
+commit 3915886843a1d4b21bd2539e7e9d4057965a52dd
+Author: Andrew Deason <adeason@sinenomine.net>
+Date: Mon Sep 16 14:06:53 2019 -0500
+
+ OPENAFS-SA-2019-003: ubik: Avoid unlocked ubik_currentTrans deref
+
+ Currently, SVOTE_Debug/SVOTE_DebugOld examine some ubik internal state
+ without any locks, because the speed of these functions is more
+ important than accuracy.
+
+ However, one of the pieces of data we examine is ubik_currentTrans,
+ which we dereference to get ubik_currentTrans->type. ubik_currentTrans
+ could be set to NULL while this code is running, so there is a small
+ chance of this code causing a segfault, if SVOTE_Debug() is running
+ when the current transaction ends.
+
+ We only ever initialize ubik_currentTrans as a write transation (via
+ SDISK_Begin), so this check is pointless anyway. Accordingly, skip the
+ type check, and always assume that any active transaction is a write
+ transaction. This means we only ever access ubik_currentTrans once,
+ avoiding any risk of the value changing between accesses (and we no
+ longer need to dereference it, anyway).
+
+ Note that, since ubik_currentTrans is not marked as 'volatile', some C
+ compilers, with certain options, can and do assume that its value will
+ not change between accesses, and thus only fetch the pointer value once.
+ This avoids the risk of NULL dereference (and thus, crash, if pointer
+ stores/loads are atomic), but the value pointed to by ubik_currentTrans->type
+ would be incorrect when the transaction ends during the execution of
+ SVOTE_Debug().
+
+ Reviewed-on: https://gerrit.openafs.org/13915
+ Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
+ Tested-by: Benjamin Kaduk <kaduk@mit.edu>
+ (cherry picked from commit 6ec46ba7773089e1549d27a0d345afeca65c9472)
+
+ Reviewed-on: https://gerrit.openafs.org/13918
+ Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
+ Tested-by: Benjamin Kaduk <kaduk@mit.edu>
+ (cherry picked from commit 213b9dc386ff89a14379313ee8ec09280f130a29)
+
+ Change-Id: I0ddbc2309de09a629150a6b3825e1aa8bda8b910
+ Reviewed-on: https://gerrit.openafs.org/13923
+ Tested-by: BuildBot <buildbot@rampaginggeek.com>
+ Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
+
+diff --git a/src/ubik/vote.c b/src/ubik/vote.c
+index 5d75a80228..4d6ab44a20 100644
+--- a/src/ubik/vote.c
++++ b/src/ubik/vote.c
+@@ -447,10 +447,7 @@ SVOTE_Debug(struct rx_call * rxcall, struct ubik_debug * aparm)
+
+ if (ubik_currentTrans) {
+ aparm->currentTrans = 1;
+- if (ubik_currentTrans->type == UBIK_WRITETRANS)
+- aparm->writeTrans = 1;
+- else
+- aparm->writeTrans = 0;
++ aparm->writeTrans = 1;
+ } else {
+ aparm->currentTrans = 0;
+ }
+@@ -530,10 +527,7 @@ SVOTE_DebugOld(struct rx_call * rxcall,
+
+ if (ubik_currentTrans) {
+ aparm->currentTrans = 1;
+- if (ubik_currentTrans->type == UBIK_WRITETRANS)
+- aparm->writeTrans = 1;
+- else
+- aparm->writeTrans = 0;
++ aparm->writeTrans = 1;
+ } else {
+ aparm->currentTrans = 0;
+ }
--- /dev/null
+commit 213b9dc386ff89a14379313ee8ec09280f130a29
+Author: Andrew Deason <adeason@sinenomine.net>
+Date: Mon Sep 16 14:06:53 2019 -0500
+
+ OPENAFS-SA-2019-003: ubik: Avoid unlocked ubik_currentTrans deref
+
+ Currently, SVOTE_Debug/SVOTE_DebugOld examine some ubik internal state
+ without any locks, because the speed of these functions is more
+ important than accuracy.
+
+ However, one of the pieces of data we examine is ubik_currentTrans,
+ which we dereference to get ubik_currentTrans->type. ubik_currentTrans
+ could be set to NULL while this code is running, so there is a small
+ chance of this code causing a segfault, if SVOTE_Debug() is running
+ when the current transaction ends.
+
+ We only ever initialize ubik_currentTrans as a write transation (via
+ SDISK_Begin), so this check is pointless anyway. Accordingly, skip the
+ type check, and always assume that any active transaction is a write
+ transaction. This means we only ever access ubik_currentTrans once,
+ avoiding any risk of the value changing between accesses (and we no
+ longer need to dereference it, anyway).
+
+ Note that, since ubik_currentTrans is not marked as 'volatile', some C
+ compilers, with certain options, can and do assume that its value will
+ not change between accesses, and thus only fetch the pointer value once.
+ This avoids the risk of NULL dereference (and thus, crash, if pointer
+ stores/loads are atomic), but the value pointed to by ubik_currentTrans->type
+ would be incorrect when the transaction ends during the execution of
+ SVOTE_Debug().
+
+ Reviewed-on: https://gerrit.openafs.org/13915
+ Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
+ Tested-by: Benjamin Kaduk <kaduk@mit.edu>
+ (cherry picked from commit 6ec46ba7773089e1549d27a0d345afeca65c9472)
+
+ Change-Id: I634ddb27e7a8dbe5c9d1dacdc83070efa470b50b
+ Reviewed-on: https://gerrit.openafs.org/13918
+ Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
+ Tested-by: Benjamin Kaduk <kaduk@mit.edu>
+
+diff --git a/src/ubik/vote.c b/src/ubik/vote.c
+index 87cfb3911e..3a8374e88f 100644
+--- a/src/ubik/vote.c
++++ b/src/ubik/vote.c
+@@ -441,10 +441,7 @@ SVOTE_Debug(struct rx_call * rxcall, struct ubik_debug * aparm)
+
+ if (ubik_currentTrans) {
+ aparm->currentTrans = 1;
+- if (ubik_currentTrans->type == UBIK_WRITETRANS)
+- aparm->writeTrans = 1;
+- else
+- aparm->writeTrans = 0;
++ aparm->writeTrans = 1;
+ } else {
+ aparm->currentTrans = 0;
+ }
+@@ -524,10 +521,7 @@ SVOTE_DebugOld(struct rx_call * rxcall,
+
+ if (ubik_currentTrans) {
+ aparm->currentTrans = 1;
+- if (ubik_currentTrans->type == UBIK_WRITETRANS)
+- aparm->writeTrans = 1;
+- else
+- aparm->writeTrans = 0;
++ aparm->writeTrans = 1;
+ } else {
+ aparm->currentTrans = 0;
+ }
git://git.openafs.org / openafs-web.git / commitdiff