Uploaded image for project: 'Log4j 2'
  1. Log4j 2
  2. LOG4J2-3210

Not effective deserialisation controls

Attach filesAttach ScreenshotVotersWatch issueWatchersCreate sub-taskLinkCloneUpdate Comment AuthorReplace String in CommentUpdate Comment VisibilityDelete Comments
    XMLWordPrintableJSON

Details

    • Bug
    • Status: Resolved
    • Major
    • Resolution: Fixed
    • 2.15.0
    • 2.17.0
    • Core

    Description

      The new JNDILookup protection by disabling the lookups and by disabling JNDI is effective. However, since you still keep the JNDI Lookup Code around I had a look at the new restrictions, and I don’t think they are affective:

      a) there is no protection (allowed host/port) for the RMI protocol. It should probably use the same allowed hosts than LDAP does
      b) when checking the allowed class in the LDAP case you asume the class Attribute is actually the class which is serialized. This must not be true, an attacker can pretend their object is a java.lang.Stringˋ which will pass your filter. Later on when you use lookup the JNDI code will try to deserialize the object - and then it is too late.

      My suggesting would be to not use context.lookup() if not needed and/or disallow all forms of serialized data (which might not work for the JMSManager case?). Anyway, for the `${jndi:`-lookup case I can imagine only string attributes are needed anyway?

      Is it safe to allow the whole java: context and how would you restrict absolute /… names without an prefix? (At the moment I guess they will get the java:env prefix since there is no logic to skip that with leading slash?)

      Sidenote: there is also a osgi: scheme which could be useful, but also dangerous.

      BTW the disabled JNDIManager with the context==null, it should have a comment what it’s meaning is „// create unusable JNDIManager with context == null“ and I am also not sure if it is only created with this code path.. maybe better to check the the isJndiEnabled also in the constructor? (I sent a pull request with a minor rename of the isIs.. method as well).

      Attachments

        Activity

          This comment will be Viewable by All Users Viewable by All Users
          Cancel

          People

            Unassigned Unassigned
            b.eckenfels Bernd Eckenfels
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Slack

                Issue deployment