Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the security vulnerability from jackcon databind #15665

Conversation

beinan
Copy link
Contributor

@beinan beinan commented Jun 3, 2022

What changes are proposed in this pull request?

fix https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-36518

Why are the changes needed?

jackson-databind before 2.13.0 allows a Java StackOverflow exception and denial of service via a large depth of nested objects.

Does this PR introduce any user facing changes?

no

@Xenorith
Copy link
Contributor

Xenorith commented Jun 4, 2022

this unit test failure looks real:
Java 8 Integration Tests / modules: (alluxio.client.**,!alluxio.client.fs.**,!alluxio.client.cli.**,!alluxio.client.fuse.**) (pull_request) Failing after 13m — modules: (alluxio.client.**,!alluxio.client.fs.**,!alluxio.client.cli.**,!alluxio.client.fuse.**)
https://github.com/Alluxio/alluxio/runs/6734201778?check_suite_focus=true

@beinan
Copy link
Contributor Author

beinan commented Jun 4, 2022

this unit test failure looks real: Java 8 Integration Tests / modules: (alluxio.client.**,!alluxio.client.fs.**,!alluxio.client.cli.**,!alluxio.client.fuse.**) (pull_request) Failing after 13m — modules: (alluxio.client.**,!alluxio.client.fs.**,!alluxio.client.cli.**,!alluxio.client.fuse.**) https://github.com/Alluxio/alluxio/runs/6734201778?check_suite_focus=true

Yes, looks like the behavior of null/empty values changed, from '' to 'null' . I will try to fix it soon.

S3ClientRestApiTest.completeMultipartUpload:1172 expected:<CompleteMultipartUploadResult{mLocation='/bucket/object', mBucket='bucket', mKey='object', mETag='"2376cabb1349637a7e68b8a8c1db2485"', mCode='', mMessage=''}> but was:<CompleteMultipartUploadResult{mLocation='/bucket/object', mBucket='bucket', mKey='object', mETag='"2376cabb1349637a7e68b8a8c1db2485"', mCode='null', mMessage='null'}>

@beinan beinan requested a review from ZhuTopher June 4, 2022 06:47
@beinan
Copy link
Contributor Author

beinan commented Jun 4, 2022

I fix the test by adding @JsonInclude(JsonInclude.Include.NON_EMPTY) to the CompleteMultipartUploadResult, because the behavior of the null/empty value handling got changed from jackson 2.12. FasterXML/jackson-dataformat-xml#411 so I just exclude null and empty values during the serialization .

So the result is changed from

 <CompleteMultipartUploadResult><Location>/bucket/object</Location><Bucket>bucket</Bucket><Key>object</Key><ETag>"53a35d042386eecf1ba819c7668a11a5"</ETag><Code/><Message/></CompleteMultipartUploadResult>

to

<CompleteMultipartUploadResult><Location>/bucket/object</Location><Bucket>bucket</Bucket><Key>object</Key><ETag>"53a35d042386eecf1ba819c7668a11a5"</ETag></CompleteMultipartUploadResult>

The <Code/> and <Message/> are excluded when the values are null or empty.

@ZhuTopher could you help take a look? thanks!

@@ -24,6 +25,7 @@
*/
@JacksonXmlRootElement(localName = "CompleteMultipartUploadResult")
@JsonPropertyOrder({ "Location", "Bucket", "Key", "ETag" })
@JsonInclude(JsonInclude.Include.NON_EMPTY)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of the available fields for CompleteMultipartUpload seem be fields which are intended to be empty strings, so I'm okay with this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact <Code/> and <Message/> belong to error responses and not CompleteMultipartUpload directly. Again, change should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @ZhuTopher ! If you saw any abnormal ser/deser results of json or xml, just let me know, thx!

@beinan
Copy link
Contributor Author

beinan commented Jun 7, 2022

alluxio-bot, merge this please.

@alluxio-bot alluxio-bot merged commit bf6b068 into Alluxio:master Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants