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

CVE-2017-7525 jackson-databind: Deserialization vulnerability via readValue method of ObjectMapper #1723

Closed
jwedel opened this Issue Aug 4, 2017 · 20 comments

Comments

Projects
None yet
4 participants
@jwedel

jwedel commented Aug 4, 2017

There is this vulnerability (CVE-2017-7525, https://bugzilla.redhat.com/show_bug.cgi?id=1462702) in jackson-databind that allows remote code execution.

I tried to check existing issues but could not find anything related.

This vulnerability has been reported in 2.8.9 as well as all pre releases of 2.9.0. Is this actually fixed in 2.9.0 or is there a patch release planned?

@jwedel

This comment has been minimized.

Show comment
Hide comment
@jwedel

jwedel Aug 4, 2017

More information about the vulnerability and possible exploits can be found in this paper:
https://www.github.com/mbechler/marshalsec/blob/master/marshalsec.pdf?raw=true

See chapter 3.1.6.

jwedel commented Aug 4, 2017

More information about the vulnerability and possible exploits can be found in this paper:
https://www.github.com/mbechler/marshalsec/blob/master/marshalsec.pdf?raw=true

See chapter 3.1.6.

@cowtowncoder

This comment has been minimized.

Show comment
Hide comment
@cowtowncoder

cowtowncoder Aug 4, 2017

Member

The issue was reported as #1599 a while ago, and fixed for 2.8.9 and 2.9.0; plus backported in 2.7.9.1 and 2.6.7.1.
Also note #1680 for further inclusion of com.sun.rowset.JdbcRowSetImpl, addressing one issue remaining after 1599 fix (included in upcoming 2.8.10, and 2.9.0).

More work is probably needed to refine set of classes blocked: about 10 or so are blacklisted, based on reports of known vulnerabilities.

Member

cowtowncoder commented Aug 4, 2017

The issue was reported as #1599 a while ago, and fixed for 2.8.9 and 2.9.0; plus backported in 2.7.9.1 and 2.6.7.1.
Also note #1680 for further inclusion of com.sun.rowset.JdbcRowSetImpl, addressing one issue remaining after 1599 fix (included in upcoming 2.8.10, and 2.9.0).

More work is probably needed to refine set of classes blocked: about 10 or so are blacklisted, based on reports of known vulnerabilities.

@jwedel

This comment has been minimized.

Show comment
Hide comment
@jwedel

jwedel Aug 5, 2017

Hi @cowtowncoder, thanks for your reply.

This vulnerability has been reported in 2.8.9 as well as all pre releases of 2.9.0.

We are using automated vulnerability scan from Sonatype called CLM. From last wednesday on, it also reported the same vulnerability also for the newer versions (see quote above). This is probably due to the black listing approach. I don't have any reference that explains exploits for those versions but I assumed that you guys probably already know.

jwedel commented Aug 5, 2017

Hi @cowtowncoder, thanks for your reply.

This vulnerability has been reported in 2.8.9 as well as all pre releases of 2.9.0.

We are using automated vulnerability scan from Sonatype called CLM. From last wednesday on, it also reported the same vulnerability also for the newer versions (see quote above). This is probably due to the black listing approach. I don't have any reference that explains exploits for those versions but I assumed that you guys probably already know.

@cowtowncoder

This comment has been minimized.

Show comment
Hide comment
@cowtowncoder

cowtowncoder Aug 6, 2017

Member

@jwedel it is also possible that whoever entered data into system did not know, perhaps since fix predates creation of CVE.

It is also unfortunate that there is no real way to indicate the fact that vulnerability requires active acts by users of the library -- without either explicitly enabling "default typing" or explicitly annotating an Object-valued property to use classname-based polymorphic typing -- which is one key consideration. One possible interpretation is that there is no way to "fix" this short of removing feature itself as use of white-listing is unlikely to make much sense for usage (users who want to limit what classes this should work on should not even use classname as id at all, but type name.

Re-reading the paper to look for other classes that should be blacklisted: all JDK-included types for which exploits reported are handled (as far as I can see), but there seem to be some from common frameworks/libs that are not.
But one exploit listed (com.sun.rowset.JdbcRowSetImpl) was actually blocked as per #1680.

Member

cowtowncoder commented Aug 6, 2017

@jwedel it is also possible that whoever entered data into system did not know, perhaps since fix predates creation of CVE.

It is also unfortunate that there is no real way to indicate the fact that vulnerability requires active acts by users of the library -- without either explicitly enabling "default typing" or explicitly annotating an Object-valued property to use classname-based polymorphic typing -- which is one key consideration. One possible interpretation is that there is no way to "fix" this short of removing feature itself as use of white-listing is unlikely to make much sense for usage (users who want to limit what classes this should work on should not even use classname as id at all, but type name.

Re-reading the paper to look for other classes that should be blacklisted: all JDK-included types for which exploits reported are handled (as far as I can see), but there seem to be some from common frameworks/libs that are not.
But one exploit listed (com.sun.rowset.JdbcRowSetImpl) was actually blocked as per #1680.

@jwedel

This comment has been minimized.

Show comment
Hide comment
@jwedel

jwedel Aug 7, 2017

@cowtowncoder hi, I've checked the source: National Vulnerability Database
http://web.nvd.nist.gov/view/vuln/detail?vulnId=CVE-2017-7525

However, as you may have tried youself, this link is a dead end. I tried to seach for the CVE and couldn't find anything. Maybe the NVD revoked that?

jwedel commented Aug 7, 2017

@cowtowncoder hi, I've checked the source: National Vulnerability Database
http://web.nvd.nist.gov/view/vuln/detail?vulnId=CVE-2017-7525

However, as you may have tried youself, this link is a dead end. I tried to seach for the CVE and couldn't find anything. Maybe the NVD revoked that?

@cowtowncoder

This comment has been minimized.

Show comment
Hide comment
@cowtowncoder

cowtowncoder Aug 7, 2017

Member

@jwedel Hmmh. Well that is interesting. Bit difficult to add more information or notes for non-public entry. :-/

Member

cowtowncoder commented Aug 7, 2017

@jwedel Hmmh. Well that is interesting. Bit difficult to add more information or notes for non-public entry. :-/

@jwedel

This comment has been minimized.

Show comment
Hide comment
@jwedel

jwedel Aug 8, 2017

@cowtowncoder Sonatype is currently checking their database to find out if it's a false positive: https://issues.sonatype.org/browse/CLMDATA-4117

You need an account to view the issue.

jwedel commented Aug 8, 2017

@cowtowncoder Sonatype is currently checking their database to find out if it's a false positive: https://issues.sonatype.org/browse/CLMDATA-4117

You need an account to view the issue.

@cowtowncoder

This comment has been minimized.

Show comment
Hide comment
@cowtowncoder

cowtowncoder Aug 8, 2017

Member

@jwedel I do have an account, but I don't seem to have access. Account id same as my handle here.

Member

cowtowncoder commented Aug 8, 2017

@jwedel I do have an account, but I don't seem to have access. Account id same as my handle here.

@cowtowncoder

This comment has been minimized.

Show comment
Hide comment
@cowtowncoder

cowtowncoder Aug 8, 2017

Member

After reviewing the document, I think there are 5 potential problem remaining: 4.8, 4.9, 4.10, 4.12 and 4.21. Of these, 4.9 seems severe, others not necessarily of much threat (like 4.21). All of these require existence of certain libs beyond JDK (unlike types blocked so far), but they are common enough to be of concern. Will need to figured out fully-qualified classnames, can then add to blacklist.

Member

cowtowncoder commented Aug 8, 2017

After reviewing the document, I think there are 5 potential problem remaining: 4.8, 4.9, 4.10, 4.12 and 4.21. Of these, 4.9 seems severe, others not necessarily of much threat (like 4.21). All of these require existence of certain libs beyond JDK (unlike types blocked so far), but they are common enough to be of concern. Will need to figured out fully-qualified classnames, can then add to blacklist.

@jwedel

This comment has been minimized.

Show comment
Hide comment
@jwedel

jwedel Aug 23, 2017

Hi @cowtowncoder,

For the documentation puposes, those vulnerabilites are potentially applicable if you have the following libraries on your classpath:

  • 4.8: c3p0
  • 4.9: c3p0
  • 4.10: spring-beans and spring-context
  • 4.12: spring-aop
  • 4.21: JDK

So 4.10 and 4.12 is very common for modern spring applications, especially spring boot.

Two things I want to clarify:

First, which versions of jackson are affected by those potential problems you mentioned? I guess it's 2.8.9 as well as 2.9.0? Are you actually planning to fix that somehow?

Second, Redhat actually claimed that they "fixed" it ( See here) although they did not tell how and I don't see any commits linked. Do you have any idea how they did fix it?

jwedel commented Aug 23, 2017

Hi @cowtowncoder,

For the documentation puposes, those vulnerabilites are potentially applicable if you have the following libraries on your classpath:

  • 4.8: c3p0
  • 4.9: c3p0
  • 4.10: spring-beans and spring-context
  • 4.12: spring-aop
  • 4.21: JDK

So 4.10 and 4.12 is very common for modern spring applications, especially spring boot.

Two things I want to clarify:

First, which versions of jackson are affected by those potential problems you mentioned? I guess it's 2.8.9 as well as 2.9.0? Are you actually planning to fix that somehow?

Second, Redhat actually claimed that they "fixed" it ( See here) although they did not tell how and I don't see any commits linked. Do you have any idea how they did fix it?

@cowtowncoder

This comment has been minimized.

Show comment
Hide comment
@cowtowncoder

cowtowncoder Aug 23, 2017

Member

@jwedel Thank you, yes, those are the libraries. Issue I added for remaining types is #1737, and will be in 2.8.10 and 2.9.1 wrt types other than JdbcRowSetImpl (and Xalan transformer impl) that were included earlier in 2.8.9 / 2.9.0.

As to Red Hat, two main possibilities I think: either (a) they were referring to #1599 (adding mechanism to prevent issues and blocking first 2 reported types) or (b) they extended the fix itself and have their own patch. I have noticed occasional red hat specific maven jars via mvnrepository, although don't think I saw one for 2.8.9.

I hope to release 2.8.10 quite soon now (have been hoping to batch up more fixes since it's potentially the last 2.8 patch), and 2.9.1 within maybe 2 weeks or so (similarly hoping to batch more fixes, not many yet for 2.9).

Member

cowtowncoder commented Aug 23, 2017

@jwedel Thank you, yes, those are the libraries. Issue I added for remaining types is #1737, and will be in 2.8.10 and 2.9.1 wrt types other than JdbcRowSetImpl (and Xalan transformer impl) that were included earlier in 2.8.9 / 2.9.0.

As to Red Hat, two main possibilities I think: either (a) they were referring to #1599 (adding mechanism to prevent issues and blocking first 2 reported types) or (b) they extended the fix itself and have their own patch. I have noticed occasional red hat specific maven jars via mvnrepository, although don't think I saw one for 2.8.9.

I hope to release 2.8.10 quite soon now (have been hoping to batch up more fixes since it's potentially the last 2.8 patch), and 2.9.1 within maybe 2 weeks or so (similarly hoping to batch more fixes, not many yet for 2.9).

@cowtowncoder cowtowncoder added this to the 2.8.10 milestone Aug 23, 2017

@OlivierJaquemet

This comment has been minimized.

Show comment
Hide comment
@OlivierJaquemet

OlivierJaquemet Sep 15, 2017

2.8.10 was release on 24 aug 2017 : https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.8.10

Shouldn't this issue be closed ? and the 2.8.10 milestone marked as completed (at the specified date) ?

OlivierJaquemet commented Sep 15, 2017

2.8.10 was release on 24 aug 2017 : https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.8.10

Shouldn't this issue be closed ? and the 2.8.10 milestone marked as completed (at the specified date) ?

@cowtowncoder

This comment has been minimized.

Show comment
Hide comment
@cowtowncoder

cowtowncoder Sep 15, 2017

Member

@OlivierJaquemet yes, thank you.

Member

cowtowncoder commented Sep 15, 2017

@OlivierJaquemet yes, thank you.

@cowtowncoder

This comment has been minimized.

Show comment
Hide comment
@cowtowncoder

cowtowncoder Sep 15, 2017

Member

Closed as duplicate of #1737

Member

cowtowncoder commented Sep 15, 2017

Closed as duplicate of #1737

@swarupe04

This comment has been minimized.

Show comment
Hide comment
@swarupe04

swarupe04 Nov 10, 2017

Basing on details from the Java Unmarshaller Security doc for Jackson, it seems to apply only for Jackson 2.x version. If we're using Jackson 1.x, guessing it doesn't require any patch. Is my understanding correct?

swarupe04 commented Nov 10, 2017

Basing on details from the Java Unmarshaller Security doc for Jackson, it seems to apply only for Jackson 2.x version. If we're using Jackson 1.x, guessing it doesn't require any patch. Is my understanding correct?

@cowtowncoder

This comment has been minimized.

Show comment
Hide comment
@cowtowncoder

cowtowncoder Nov 10, 2017

Member

@swarupe04 No, it would apply to 1.x as well, mechanism exists there as well.

Member

cowtowncoder commented Nov 10, 2017

@swarupe04 No, it would apply to 1.x as well, mechanism exists there as well.

@OlivierJaquemet

This comment has been minimized.

Show comment
Hide comment
@OlivierJaquemet

OlivierJaquemet Nov 11, 2017

@cowtowncoder interesting, thus was it fixed in a specific release of 1.x ?

  • if yes, which one ?
  • if no do you plan do to so ?

OlivierJaquemet commented Nov 11, 2017

@cowtowncoder interesting, thus was it fixed in a specific release of 1.x ?

  • if yes, which one ?
  • if no do you plan do to so ?
@cowtowncoder

This comment has been minimized.

Show comment
Hide comment
@cowtowncoder

cowtowncoder Nov 12, 2017

Member

@OlivierJaquemet No it has not been fixed to my knowledge in github repo. And even if it was there is no functioning mechanism for making 1.x releases; all were done from and via Codehaus.

It is possible to write a module to do equivalent of fixes in 2.x (as this just requires blocking of deserialization of certain types). I also do not know if there are any 1.x users using default typing.

I don't have any plans personally to work on this, but as usual will assist if someone with an itch is willing to work on it (including Maven release part).

Member

cowtowncoder commented Nov 12, 2017

@OlivierJaquemet No it has not been fixed to my knowledge in github repo. And even if it was there is no functioning mechanism for making 1.x releases; all were done from and via Codehaus.

It is possible to write a module to do equivalent of fixes in 2.x (as this just requires blocking of deserialization of certain types). I also do not know if there are any 1.x users using default typing.

I don't have any plans personally to work on this, but as usual will assist if someone with an itch is willing to work on it (including Maven release part).

@OlivierJaquemet

This comment has been minimized.

Show comment
Hide comment
@OlivierJaquemet

OlivierJaquemet Nov 12, 2017

Thanks for this important feedback @cowtowncoder,

At the time of this writing, current status for CVE-2017-7525 is still RESERVED, therefore the vulnerability is not yet listed in the NVD database. I supposed because it is still being addressed. Do you know if there are pending fix as far as Jackson is concerned ?

I saw you backported a security fix and updated the release note for an hypothetical 1.9.14 release FasterXML/jackson-1@d24261c
Is is the same issue ? If so does it mean the last steps required is a build and maven release ?

It is my understanding that 1.x is still used by some major software (Atlassian for example has it own fork : https://github.com/FasterXML/jackson-1 , with bits added to provide published release on its maven repo https://mvnrepository.com/artifact/org.codehaus.jackson/jackson-mapper-lgpl). Maybe they can provide help on this matter? @miszobi, @marcins, do you maintain Atlassian fork ? Would you be interested in publishing a -4 release with this security fix ?

OlivierJaquemet commented Nov 12, 2017

Thanks for this important feedback @cowtowncoder,

At the time of this writing, current status for CVE-2017-7525 is still RESERVED, therefore the vulnerability is not yet listed in the NVD database. I supposed because it is still being addressed. Do you know if there are pending fix as far as Jackson is concerned ?

I saw you backported a security fix and updated the release note for an hypothetical 1.9.14 release FasterXML/jackson-1@d24261c
Is is the same issue ? If so does it mean the last steps required is a build and maven release ?

It is my understanding that 1.x is still used by some major software (Atlassian for example has it own fork : https://github.com/FasterXML/jackson-1 , with bits added to provide published release on its maven repo https://mvnrepository.com/artifact/org.codehaus.jackson/jackson-mapper-lgpl). Maybe they can provide help on this matter? @miszobi, @marcins, do you maintain Atlassian fork ? Would you be interested in publishing a -4 release with this security fix ?

@cowtowncoder

This comment has been minimized.

Show comment
Hide comment
@cowtowncoder

cowtowncoder Nov 13, 2017

Member

@OlivierJaquemet Ah. Look at that. My memory is like sieve sometimes. This was the PR of contribution:

FasterXML/jackson-1#5

and I indeed merged as you point out.

And yes, only Maven release part would be needed. Releases for 1.x used Maven Ant tasks and last I looked it didn't look like task was supported or working well. Conversion to Maven was not a trivial task; I would not be against conversion itself at all but back when 1.x was being developed it seemed unnecessary.

One challenge with conversion was, I think, production of license-specific jars (asl / lgpl).
I don't think production of 2 jars would be mandatory here, although question then would be which one to produce (wrt naming): my guess is that asl variant is more likely choice (and it definitely has higher d/l counts via Maven Central).

If build process can be fixed I'd be happy to push the release, so any help would be useful.

About the only misgiving I have is that it is not clear that any of 1.x users are actually using vulnerable settings. But then again ability to push a security patch for 1.x would be valuable regardless of actual real need for this specific issue.

Member

cowtowncoder commented Nov 13, 2017

@OlivierJaquemet Ah. Look at that. My memory is like sieve sometimes. This was the PR of contribution:

FasterXML/jackson-1#5

and I indeed merged as you point out.

And yes, only Maven release part would be needed. Releases for 1.x used Maven Ant tasks and last I looked it didn't look like task was supported or working well. Conversion to Maven was not a trivial task; I would not be against conversion itself at all but back when 1.x was being developed it seemed unnecessary.

One challenge with conversion was, I think, production of license-specific jars (asl / lgpl).
I don't think production of 2 jars would be mandatory here, although question then would be which one to produce (wrt naming): my guess is that asl variant is more likely choice (and it definitely has higher d/l counts via Maven Central).

If build process can be fixed I'd be happy to push the release, so any help would be useful.

About the only misgiving I have is that it is not clear that any of 1.x users are actually using vulnerable settings. But then again ability to push a security patch for 1.x would be valuable regardless of actual real need for this specific issue.

miguel-gr added a commit to miguel-gr/currency-converter that referenced this issue Oct 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment