Document Security Policy#9730
Conversation
|
FYI @scovich @Jefffrey @etseidl @tustvold @jhorstmann, and @crepererum who may be interested in commenting on this policy |
scovich
left a comment
There was a problem hiding this comment.
The doc and associated link sprinkling seem very reasonable and welcome, but I don't think I understand how this will "reduce the flow" of "a deluge of low quality bug reports masquerading as security problems" ?
Or does it just make it easier to summarily close them because they don't follow the official guidelines?
❤️
Yes, basically having this documented means that upstream filters (aka the apache security team) can point to these guidelines if they get reports that are bugs. At the moment in Arrow sometimes it is not clear which crash is a security issue and which is just a bug
Yes that is an excellent idea and I will do so |
| [`simdutf8`]: https://crates.io/crates/simdutf8 | ||
| [parquet variant]: https://github.com/apache/parquet-format/blob/master/VariantEncoding.md | ||
|
|
||
| ## Parquet Feature Status |
There was a problem hiding this comment.
Why is this section removed?
There was a problem hiding this comment.
🤦 -- not sure -- will revert
Co-authored-by: Ed Seidl <etseidl@users.noreply.github.com>
| //! malformed input is considered a **bug**, not a security vulnerability, | ||
| //! unless it is **exploitable** by an attacker to | ||
| //! | ||
| //! * Execute arbitrary code (Remote Code Execution); |
There was a problem hiding this comment.
I added explicit definition of what exploitable means here. I did not include a "Denial Of Service" per the discussion with @tustvold on https://github.com/apache/arrow/pull/49761/changes#r3087975701
| //! Like many crates, this crate makes use of `unsafe` where prudent. However, it endeavors to be | ||
| //! sound. Specifically, **it should not be possible to trigger undefined behavior using safe APIs.** | ||
| //! | ||
| //! For more information on the use of unsafe, see [here](https://github.com/apache/arrow-rs/tree/main/arrow#safety). |
There was a problem hiding this comment.
Should a soundness issue be considered a security issue or not? I mainly ask as whilst we want to encourage reporting of soundness issues, they aren't in and of themselves exploitable.
There was a problem hiding this comment.
I suggest we treat them as a security "issue" but not treated as a security vulnerability that requires special reporting / alerting unless there is some evidence it can actually be exploited
I tried to clarify this in the "security" part of arrow/README
Unexpected behavior (e.g., panics, crashes, or infinite loops) triggered by
malformed input, and instances of undefined behavior (UB) triggered via safe
APIs are considered bugs rather than security vulnerabilities unless they are exploitable
by an attacker to
I do agree this leaves some interpretation about what "exploitable by an attacker" really means but I think some level of interpretation is unavoidable for undefined behavior in principle (as it is undefined)
I will add a note to the safety section referring to the security section to make this explicit
There was a problem hiding this comment.
I added more explanation to SECURITY.md and left links to there
| If that exploitation path is unclear, the issue should likely be reported as a | ||
| bug. | ||
|
|
||
| ## Rust Safety, Soundness, and Undefined Behavior |
There was a problem hiding this comment.
I added this section to try and help define that not all soundness issues are vulnerabilities. I think it makes our current practice explicit.
wjones127
left a comment
There was a problem hiding this comment.
This is great. I have two non blocking comments.
| malformed input is generally considered a **bug**, not a security | ||
| vulnerability, unless it is **exploitable** and could allow an attacker to | ||
|
|
||
| * Execute arbitrary code (Remote Code Execution); |
There was a problem hiding this comment.
It might be nice to give an example of past bugs that indicate each, if there are any.
There was a problem hiding this comment.
Thankfully (?) I am not aware of any such bugs in arrow-rs so I don't have any to list
I had codex find past examples, and the ones it found are as follows, which are mostly old undefined behavior issues
- Out of bound reads in chunk iterator #198
- Undefined behavior in FFI implementation #322
- Soundness: Creating
BinaryArrayfromArrayDatadoes not perform bound checks on reading values #772 - Soundness: Creating
BinaryArrayfromArrayDatadoes not perform bound checks on reading offsets #773 - Soundness: Creating
GenericStringArrayfromArrayDatadoes not perform bound checks on reading offsets #776 - Soundness: reading parquet with invalid utf8 results in UB #786
- Validate arguments to ArrayData::try_new() #817
- Tracking issue for safety issues #221
- Upgrade
regexdependency #1874 - Implement full validation for
UnionArraysconstruction from ArrayData #1486 BitmapLength Validation is Incorrect #1231- Undefined behavior for
GenericStringArray::from_iter_valuesif reported iterator upper bound is incorrect #1144
| - Reading data from untrusted sources (e.g., over a network or from a file) requires explicit validation. | ||
| - Failure to validate untrusted data before use may lead to security issues. | ||
|
|
||
| This implementation provides APIs such as [`ArrayData::validate_full`] to |
There was a problem hiding this comment.
Is a gap in these kinds of methods a security issue then?
There was a problem hiding this comment.
Yes I think any gap in this validation would be a bug. If such a gap that allowed malformed data to be read without error and exposed RCE or information, I would think we should treat it like any other security vulnerability.
Following the same critera for security evaluation , it would only be a security vulnerability if it allows an attacker RCE / information disclosure
|
Ok, I think this is looking good enough to me, so merging it in and we can iterate on it going forward. Thank you all |
Which issue does this PR close?
Rationale for this change
Other arrow subprojects (C++ in particular) has been beset recently by a deluge of low quality bug reports masquerading as security problems.
To reduce this flow and make it easier to direct people to the appropriate bug vs feature venue, we should document our security posture better
What changes are included in this PR?
Are these changes tested?
By CI
Are there any user-facing changes?
yes, new policyt