Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8345139

Fix bugs and inconsistencies in the Provider services map

XMLWordPrintable

    • Icon: Bug Bug
    • Resolution: Unresolved
    • Icon: P4 P4
    • None
    • None
    • security-libs
    • None

      The java.security.Provider class provides a mechanism to register and retrieve services that can be used to create instances of service implementations (SPIs). SPIs typically contain cryptographic algorithms that are the backbone of service APIs such as MessageDigest, Cipher, Signature, Mac, etc.

      For historic reasons probably related to provider configuration and serialization, the class java.security.Provider inherits from java.util.Properties and services can be registered by adding, modifying or removing properties in the map. Service registration with java.security.Provider::put is an example of how a properties API was assigned special semantics, not only to store a key-value pair but to create a service instance underneath. As services required to represent and retain more information (e.g. aliases and attributes), properties keys (Strings) were shaped to follow a special syntax with reserved prefixes (e.g. "Alg.Alias"). This design proved to be inconvenient, not ready for concurrency, error-prone and a poor choice overall: the services domain is different from the properties one for the relationship between these classes to be of inheritance. For these reasons, a new API (::putService, ::getService, ::removeService, etc) that breaks apart from the idea of a properties map was introduced, representing services as instances of the public class java.security.Provider.Service. In the interest of compatibility, both APIs were adjusted for coexistence.

      Coexistence between the legacy and new APIs required synchronization between a properties map and an internal service objects map. This synchronization has multiple bugs and inconsistencies, some introduced in the original implementation and some later when fixing performance bottlenecks. In the context of this task we will explore these problems and aim to fix them, without altering neither the new nor the legacy API.


      Existing bugs and inconsistencies
      ==========================

      While analyzing the existing services map implementation, we categorized bugs found in 3 sets: 1) Concurrency bugs, 2) Serialization consistency bugs, and 3) Other bugs. While many of these bugs are related to corner cases that are unlikely to be triggered in real providers, others can potentially happen in highly concurrent scenarios and would be difficult to reproduce.

      1 - Concurrency bugs
      ~~~~~~~~~~~~~~~~~

      1.1 - A concurrent Provider::getServices call may remove services that are temporarily considered invalid because they are in the midst of an update. I.e. a provider added an alias but not the class name still, and a reader removes the service in between:

      Thread-1 (writer): Provider::put("Alg.Alias.MessageDigest.alias1", "alg1") ---> An invalid service is implicitly created
      Thread-2 (reader): Provider::getServices() ---> The invalid service is removed
      Thread-1 (writer): Provider::put("MessageDigest.alg1", "class1") ---> While the service is added again (valid, now), it won't have alias1.

      While the situation sounds unlikely for a regular service registration, it is possible when deserializing Providers as the order in which Property map entries are visited is not guaranteed.

      This scenario was not possible before JDK-8276660 because readers only removed invalid entries from legacyMap, but not from legacyStrings. As a result, invalid services could land in legacyMap without losing data after they become valid. See a reference to the JDK-8276660 removal here [1].

      Notice that the fact that legacyMap is a ConcurrentHashMap instance does not prevent this bug from happening, as the problem is between non-atomic and non-synchronized writes.

      1.2 - This bug is similar to 1.1 but occurs in Provider::getService [2].

      1.3 - When signaling legacyChanged = false after recomputing the services set here [3], a concurrent legacyMap update that right before set legacyChanged = true [4] [5] would be missed. This was introduced by JDK-8276660 because, previously, legacyChanged = false was done in ensureLegacyParsed and this method was called in a synchronized getService block. Notice here that making legacyChanged volatile did not prevent this issue, as the problem is that publishing the new computed set and resetting the legacyChanged variable is not an atomic operation and a new update could have happened in between. We think that this type of problem can be solved in a lock-free way with a pattern such as the one proposed in ServicesMap::getServicesSet, that includes a double compare and exchange with a placeholder while computing the set.

      1.4 - In operations such as Provider::implReplaceAll, there is a window in which all services look gone for readers [6] and this may cause spurious NoSuchAlgorithmException exceptions. Before JDK-8276660 the operation was seen as atomic by readers. The replaceAll change in the Properties map was made on legacyStrings by writers but only visible after readers impacted changes under a synchronized block. We think that replaceAll and putAll should be atomic for readers. In particular, putAll would be the legacy API mechanism to ensure that readers see services only when they have all their attributes added, and there is no risk of obtaining a service with half of them.

      1.5 - When a reader obtains a service from the legacyMap, Service::newInstance or Service::supportsParameter may be invoked and cached values such as classCache, constructorCache, hasKeyAttributes, supportedFormats and supportedClasses set. If there are further changes to the service (i.e.: new attributes added), cached values are never invalidated and there is a single service instance. As a result, the service information becomes inconsistent and the new attributes are missed, even for new readers. This did not happen before JDK-8276660 because ensureLegacyParsed started with a clear legacyMap and new Service instances (with uninitialized cache fields) were added.

      2 - Serialization consistency bugs
      ~~~~~~~~~~~~~~~~~~~~~~~~~~

      This type of bugs make a deserialized Provider to have different services (class names, aliases and attributes) than the original instance. What they have in common is one or more of the following traits: 1) lack of synchronization between the Properties map and the actual inner services map, 2) an incorrect assumption that the Properties map is visited, while deserializing, in the same order in which entries were originally added, or 3) an inconsistent collision behavior between the Provider::put and Provider::putService APIs.

      We will show some examples that, while unrealistic, serve for the purpose of illustrating the aforementioned inconsistencies:

      2.1 - Case-sensitive entries

      Ordered list of actions:

      put("MessageDigest.alg", "class1")
      put("MessageDigest.ALG", "class2")

      The previous list of actions creates a single Service instance that has "class2" as its class name. In the Properties map, however, both entries are present.

      List of actions in the order that they are executed while deserializing the provider:

      put("MessageDigest.ALG", "class2")
      put("MessageDigest.alg", "class1")

      The created Service instance, after deserialization, has "class1" as its class name.

      2.2 - Entries by alias

      Ordered list of actions:

      put("MessageDigest.alg", "class1")
      put("Alg.Alias.MessageDigest.alias", "alg")
      put("Alg.Alias.MessageDigest.alias2", "alias")

      The previous list of actions creates a single Service instance that has "alg" as its algorithm, and "alias" and "alias2" as its aliases.

      List of actions in the order that they are executed while deserializing the provider:

      put("Alg.Alias.MessageDigest.alias2", "alias")
      put("Alg.Alias.MessageDigest.alias", "alg")
      put("MessageDigest.alg", "class1")

      After deserialization there will be two Service instances: one has "alias" as its algorithm and "alias2" as its alias, and the other has "alg" as its algorithm and "alias" as its alias. The former instance is invalid and reachable by "alias2" only, and the latter is valid and reachable by "alg" or "alias".

      2.3 - Algorithm case-insensitive collision between Provider::putService and Provider::put

      Ordered list of actions:

      put("MessageDigest.ALG", "class1")
      putService(new Service(provider, "MessageDigest", "alg", "class2", null, null))

      The previous list of actions creates two Service instances, from which the one using "class2" as its class name is visible. However, the Properties map has entries for both services.

      List of actions in the order that they are executed while deserializing the provider:

      put("MessageDigest.alg", "class2")
      put("MessageDigest.ALG", "class1")

      After deserialization, only the Service instance that has "class1" as its class name is available.

      2.4 - Alias collision between Provider::putService and Provider::put

      putService(new Service(provider, "MessageDigest", "alg1", "class1", List.of("alias"), null))
      put("MessageDigest.alg2", "class2")
      put("Alg.Alias.MessageDigest.alias", "alg2")

      The previous list of actions creates two service instances, from which the one using "class1" as its class name is visible by "alias". However, the Properties map has the entry "Alg.Alias.MessageDigest.alias" associated with the service instance using "class2" as its class name.

      List of actions executed while deserializing the provider (in any order):

      put("MessageDigest.alg1", "class1")
      put("MessageDigest.alg2", "class2")
      put("Alg.Alias.MessageDigest.alias", "alg2")

      After deserialization, the Service instance using "class2" as its class name is the one identified by the alias "alias".

      This same inconsistency may occur with the algorithm instead of the alias.

      2.5 - Overwrites of algorithms with aliases

      Ordered list of actions:

      put("MessageDigest.alg1", "class1")
      put("Alg.Alias.MessageDigest.alias1", "alg1")
      put("MessageDigest.alg2", "class2")
      put("Alg.Alias.MessageDigest.alg1", "alg2")

      The previous list of actions creates two service instances, one that has "alg1" as its algorithm, "class1" as its class name and "alias1" as its alias, and the other that has "alg2" as its algorithm, "class2" as its class name and "alg1" as its alias.

      List of actions in the order that they are executed while deserializing the provider:

      put("MessageDigest.alg2", "class2")
      put("Alg.Alias.MessageDigest.alg1", "alg2")
      put("MessageDigest.alg1", "class1")
      put("Alg.Alias.MessageDigest.alias1", "alg1")

      After deserialization, a single Service instance is created. This instance has "alg2" as its algorithm, "class1" as its class name, and "alg1" and "alias1" as its aliases.

      3 - Other bugs
      ~~~~~~~~~~~

      3.1 - Invalid services (without a class name) can be returned in Provider::getService, even when they are removed from the legacyMap [7].

      3.2 - Methods Provider::implMerge and Provider::implCompute pass a "null" value as the 2nd parameter to Provider::parseLegacy and a remove action [8] [9]. In the case of an alias, Provider::parseLegacy will throw a NullPointerException here [10]. This issue has been introduced by JDK-8276660.

      Note: we might be overlooking more bugs, as we decided to go with a new implementation at this point.


      [1] https://github.com/openjdk/jdk/blob/jdk-25+0/src/java.base/share/classes/java/security/Provider.java#L1192
      [2] https://github.com/openjdk/jdk/blob/jdk-25+0/src/java.base/share/classes/java/security/Provider.java#L1149
      [3] https://github.com/openjdk/jdk/blob/jdk-25+0/src/java.base/share/classes/java/security/Provider.java#L1200
      [4] https://github.com/openjdk/jdk/blob/jdk-25+0/src/java.base/share/classes/java/security/Provider.java#L1005
      [5] https://github.com/openjdk/jdk/blob/jdk-25+0/src/java.base/share/classes/java/security/Provider.java#L1041
      [6] https://github.com/openjdk/jdk/blob/jdk-25+0/src/java.base/share/classes/java/security/Provider.java#L836
      [7] https://github.com/openjdk/jdk/blob/jdk-25+0/src/java.base/share/classes/java/security/Provider.java#L1145-L1161
      [8] https://github.com/openjdk/jdk/blob/jdk-25+0/src/java.base/share/classes/java/security/Provider.java#L859
      [9] https://github.com/openjdk/jdk/blob/jdk-25+0/src/java.base/share/classes/java/security/Provider.java#L876
      [10] https://github.com/openjdk/jdk/blob/jdk-25+0/src/java.base/share/classes/java/security/Provider.java#L1006

            fferrari Francisco Ferrari Bihurriet
            mbalao Martin Balao Alonso
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Created:
              Updated: