授權陷阱：Keycloak 隱藏了什麼？

作者：瓦萊裡‧菲拉托夫

用戶授權和註冊是任何應用程式的重要組成部分，不僅是為了用戶，也是為了安全。流行的開源身分管理解決方案的原始碼隱藏了哪些陷阱？它們對申請有何影響？

如果您曾經為網頁應用程式實施授權，您可能知道可能出現的所有令人沮喪的問題。我也不例外。

有一次，我在一個專案的前端部分實現了基於Messenger的授權。這似乎是世界上最簡單的任務，但事實卻恰恰相反：最後期限迫在眉睫，程式碼被訊息 API 絆倒，我周圍的人都在大喊大叫。

在這個案例之後，我的同事向我展示了一個很酷的工具，可以簡化我們未來專案中授權的實施。這個專案是 Keycloak，一個開源 Java 解決方案，可實現單一登入 (SSO) 以及針對現代應用程式和服務的身份和存取管理。

當我自己使用該解決方案時，我發現進入原始程式碼並使用 PVS-Studio 靜態分析器來查找隱藏在那裡的錯誤很有趣。

呵呵，經典的NullPointerException

有人敲門。初級開發人員試圖打開一扇門，結果摔壞了。 「空指標異常！」資深開發人員總結

與檢查 null 相關的錯誤幾乎在我們之前檢查過的每個項目中都會遇到。那麼，讓我們從這個古老但黃金的錯誤開始。

片段 N1

private void checkRevocationUsingOCSP(X509Certificate[] certs) 
  throws GeneralSecurityException {
    ....    
    if (rs == null) {
      if (_ocspFailOpen)
        logger.warnf(....);
      else
        throw new GeneralSecurityException(....);
    }

    if (rs.getRevocationStatus() == 
      OCSPProvider.RevocationStatus.UNKNOWN) { // <=
        if (_ocspFailOpen)
            logger.warnf(....);
        else
            throw new GeneralSecurityException(....);
    ....
}

警告：

V6008 'rs' 的潛在空取消引用。

此程式碼片段有一個 null 檢查，但我們需要了解內部發生了什麼。如果 _oscpFailOpen 變數為 true，程式不會拋出例外。它只會保存有關它的日誌並繼續執行- 它會在以下if 中收到NullPointerException，因為rs 變數已在如果。

如果沒有 NullPointerException，程式可能會拋出另一個例外。但事實並非如此，因為 _oscpFailOpen 變數為 true，程式應該繼續執行。沒緣分，它被null指針絆倒，陷入了異常。

片段 N2

public void writeDateAttributeValue(XMLGregorianCalendar attributeValue) 
  throws ProcessingException {
    ....
    StaxUtil.writeAttribute(
      writer, 
      "xsi", 
      JBossSAMLURIConstants.XSI_NSURI.get(), 
      "type", 
      "xs:" + attributeValue.getXMLSchemaType().getLocalPart()   // <=
    ); 

    if (attributeValue == null) {
      StaxUtil.writeAttribute(
        writer, 
        "xsi", 
        JBossSAMLURIConstants.XSI_NSURI.get(), 
        "nil", 
        "true"
      );
    ....
}

警告*：*

V6060 在驗證「attributeValue」是否為 null 之前，已使用了「attributeValue」參考。

“遲到總比不到好”，這句話對於很多情況來說已經足夠了，但不幸的是，不適用於 null 檢查。在上面的程式碼片段中，attributeValue物件在檢查其是否存在之前被使用，這導致了NullPointerException。

注意。如果您想查看此類錯誤的其他範例，我們已為您整理了一個清單！

論據？

我不知道這是怎麼發生的，但 Keycloak 存在與格式字串函數中的參數數量相關的錯誤。這不是一個說明性的統計數據，只是一個有趣的事實。

片段 N3

protected void process() {
  ....
  if (f == null) {
    ....
  } else {
    ....
    if (isListType(f.getType()) && t instanceof ParameterizedType) {
      t = ((ParameterizedType) t).getActualTypeArguments()[0];
      if (!isBasicType(t) && t instanceof Class) {
        ....
        out.printf(", where value is:\n", ts);              // <=
        ....
      }
    } else if (isMapType(f.getType()) && t instanceof ParameterizedType) {
      ....
      out.printf(", where value is:\n", ts);                // <=
      ....
    }
  }
}

警告：

V6046 格式不正確。預計會有不同數量的格式項。未使用的參數：1.

在上面的片段中，當呼叫 printf 函數時，我們傳遞一個格式字串和一個要替換到該字串中的值。只有一個問題：字串中根本沒有地方可以替換參數。

非常有趣的是，開發人員不僅將if 的程式碼片段複製並貼上到else if，而且還將錯誤複製並貼上到else 中如果.

在下一個片段中，我們有相反的情況：開發人員沒有爭論。

片段 N4

public String toString() {
  return String.format(
    "AuthenticationSessionAuthNoteUpdateEvent 
      [ authSessionId=%s,
        tabId=%s,
        clientUUID=%s,
        authNotesFragment=%s ]", 
    authSessionId, 
    clientUUID, 
    authNotesFragment);      // <=
}

警告：

V6046 格式不正確。預計會有不同數量的格式項。缺少參數：4.

雖然開發人員將 authSessionId、clientUUID 和 authNotesFragment 參數傳遞給函數，但第四個 tabld 參數有點遺漏。

在這種情況下，String.format 方法將拋出 IllegalFormatException，這可能是一個令人討厭的意外。

不乾淨的程式碼

以下 PVS-Studio 警告與程式碼運作狀況錯誤無關。這些警告更多的是關於代碼的品質。我只是指出這些不是固定錯誤。

"What's the point of looking at them?" you may think. First, I believe that code should be clean and neat. It's not a matter of tastes: clean code is not only about the visual experience but also about code readability. Imho, it's important for any project, especially Open Source. Secondly, I'd like to show that the PVS-Studio static analyzer helps not only fix errors in code but also makes it beautiful and clean.

Not enough variables, My Lord!

For some reason, the following code fragment looks in my mind like an evil plan of someone who has a bad attitude to use variables: "Let's adopt variables and leave them waiting in horror for the garbage collector to gobble them up..."

Fragment N5

 private void onUserRemoved(RealmModel realm, String userId) {
    int num = em.createNamedQuery("deleteClientSessionsByUser")
      .setParameter("userId", userId).executeUpdate();             // <=
    num = em.createNamedQuery("deleteUserSessionsByUser")
      .setParameter("userId", userId).executeUpdate();
}

Warning:

V6021 The value is assigned to the 'num' variable but is not used.

From the point of program operation, nothing terrible happens in this code snippet. But it's still not clear why developers wrote that.

The num variable contains the number of set parameters that the executeUpdate method returns. So, I thought that the method might have had a check for changes. Even if I rewind in time, I'll only find that calls to the method aren't written to a variable but accept the current state a little later.

The result is a useless assignment to the variable—just like in the next fragment.

Fragment N6

private void verifyCodeVerifier(String codeVerifier, String codeChallenge, 
  String codeChallengeMethod) throws ClientPolicyException {
    ....
    String codeVerifierEncoded = codeVerifier;

    try {
      if (codeChallengeMethod != null &&
        codeChallengeMethod.equals(OAuth2Constants.PKCE_METHOD_S256)) {

        codeVerifierEncoded = generateS256CodeChallenge(codeVerifier);

        } else {
          codeVerifierEncoded = codeVerifier; // <=
        }
    } catch (Exception nae) {
      ....
    }
}

Warning:

V6026 This value is already assigned to the 'codeVerifierEncoded' variable.

If you look at the code, you can see that before this, the codeVerifierEncoded variable has already assigned the same value in the else branch. A developer just did redundant action: and useless, and overcomplicating.

The next code fragment just amuses me.

Fragment N7

private static Type[] extractTypeVariables(Map<String, Type> typeVarMap, 
  Type[] types){
    for (int j = 0; j < types.length; j++){
      if (types[j] instanceof TypeVariable){
        TypeVariable tv = (TypeVariable) types[j];
        types[j] = typeVarMap.get(tv.getName());
      } else {
        types[j] = types[j];     // <=
      }
    }
    return types;
}

Warning:

V6005 The variable 'types[j]' is assigned to itself.

It looks just like the previous snippet, but honestly, I'm totally lost on what this code is trying to do.

At first, I thought we were facing a nested loop here, and the author had just mixed up the variables i and j. But eventually I realized that there's only one loop here.

I also thought the assignment appeared when developers were refactoring the code, which might have been even more complicated before. In the end, I found that the function was originally created this way (commit).

So sweet copy-paste

Copy-paste errors are quite common. I'm sure you've seen them even in your own code.

Keycloak has some traces of the copy-paste use, too.

Fragment 8

public class IDFedLSInputResolver implements LSResourceResolver {
  ....

  static {
    ....
    schemaLocations.put("saml-schema-metadata-2.0.xsd", 
      "schema/saml/v2/saml-schema-metadata-2.0.xsd");
    schemaLocations.put("saml-schema-x500-2.0.xsd", 
      "schema/saml/v2/saml-schema-x500-2.0.xsd");
    schemaLocations.put("saml-schema-xacml-2.0.xsd", 
      "schema/saml/v2/saml-schema-xacml-2.0.xsd");

    schemaLocations.put("saml-schema-xacml-2.0.xsd", 
      "schema/saml/v2/saml-schema-xacml-2.0.xsd");          // <=

    schemaLocations.put("saml-schema-authn-context-2.0.xsd", 
      "schema/saml/v2/saml-schema-authn-context-2.0.xsd");
    ....
  }
  ....
}

Warning*:*

V6033 An item with the same key '"saml-schema-xacml-2.0.xsd"' has already been added.

Honestly, even though I knew there was a typo in the source code, I had a hard time finding it right away in the code.

If you notice, in the schemaLocations.put method calls, the passed arguments are quite similar. So, I assume that the dev who wrote this code simply copied a string as a template and then just changed values. The problem is that during copy-pasting, one line that repeats the previous one has crept into the project.

Such "typos" can either lead to serious consequences or have no effect at all. This copy-pasting example has been in the project since November 21, 2017 (commit), and I don't think it causes any serious problems.

We're including this error in the article to remind developers to be careful when copying and pasting code fragments and to keep an eye on any changes in code. Want to read more about it? Here's the article about the copy-paste typos.

Unreachable code

The headline gives us a little clue as to what kind of warning awaits us in the following snippet. I suggest you use your detective skills to spot the flaw yourself.

Fragment N9

public void executeOnEvent(ClientPolicyContext context) 
  throws ClientPolicyException {
    switch (context.getEvent()) {
      case REGISTER:
      case UPDATE:
        ....
      case RESOURCE_OWNER_PASSWORD_CREDENTIALS_REQUEST:
        ....
        executeOnAuthorizationRequest(ropcContext.getParams());
        return;
      default:
        return;
    }
}

It's not that easy to detect, isn't it? I'm not gloating over you, I just give you a chance to roughly access the situation. I show it because a small flaw makes it harder for the developer to find the error without examining the rest of the code. To find out what error is lurking in this code snippet, we need to look at what is cloaked in the executeOnAuthorizationRequest method:

private void executeOnAuthorizationRequest(MultivaluedMap<String, 
  String> params) throws ClientPolicyException {
    ....
    throw new ClientPolicyException(....);
}

Yes, this method throws an exception. That is, all the code written after calling this method will be unreachable—the PVS-Studio analyzer detected it.

public void executeOnEvent(ClientPolicyContext context) 
  throws ClientPolicyException {
    switch (context.getEvent()) {
      case REGISTER:
      case UPDATE:
        ....
      case RESOURCE_OWNER_PASSWORD_CREDENTIALS_REQUEST:
        ....
        executeOnAuthorizationRequest(ropcContext.getParams());
        return;       // <=
      default:
        return;
    }
}

Warning:

V6019 Unreachable code detected. It is possible that an error is present.

Even if this flaw is quite small, a similar case could lead to more serious consequences. I can only note here that a static analyzer will help you avoid such unpleasant things.

I dictate CONDITION

Now, let's look at conditional statements and cases when they execute their code.

Fragment N10

public boolean validatePassword(AuthenticationFlowContext context, 
  UserModel user, MultivaluedMap<String, String> inputData, 
  boolean clearUser) {

    ....

    if (password == null || password.isEmpty()) {
      return badPasswordHandler(context, user, clearUser,true);
    }

    ....

    if (password != null && !password.isEmpty() &&    // <=
      user.credentialManager()
        .isValid(UserCredentialModel.password(password))) { 
      ....
    }
}

Warning:

V6007 Expression 'password != null' is always true.

V6007 Expression '!password.isEmpty()' is always true.

Here are two warnings in one line! What does the analyzer warn us about? The first conditional statement checks that the password is non-null and non-empty. If the opposite occurs, the function is no longer executed. In the line the analyzer highlighted, both of these checks are repeated, so the conditions are always true.

On the one hand, it's better to check than not to check. On the other, such duplicates may lead to missing the part of the condition that may be equal to false—it can really affect the program operation.

Let's repeat the exercise.

Fragment N11

public KeycloakUriBuilder schemeSpecificPart(String ssp)
  throws IllegalArgumentException {
    if (ssp == null) 
      throw new IllegalArgumentException(....);
    ....
    if (ssp != null) // <=
      sb.append(ssp);
    ....
}

Warning:

V6007 Expression 'ssp != null' is always true.

In general, the case is the same. If the ssp variable is null, the program throws an exception. So, the condition below isn't only true all the time but is also redundant because the corresponding code block will always be executed.

The condition in the next fragment is also redundant.

Fragment N12

protected String changeSessionId(HttpScope session) {
  if (!deployment.turnOffChangeSessionIdOnLogin()) 
    return session.getID();     // <=
  else return session.getID();
}

Warning:

V6004 The 'then' statement is equivalent to the 'else' statement.

In this method, the same code is executed under different seasons, the moon phases, and, most importantly, under different conditions. So, again, the condition is redundant here.

Digging into the code, I found the code snippet that is like two drops of water similar to the one above:

protected String changeSessionId(HttpSession session) {
  if (!deployment.turnOffChangeSessionIdOnLogin()) 
    return ChangeSessionId.changeSessionId(exchange, false);
  else return session.getId();
}

As you can see, when the condition is executed, the method that changes the session ID is called.

So, we can make two guesses: either devs just copied the code and changed the condition result, or the first condition still should have updated the session, and this error goes way beyond the "sloppy" code.

But we mustn't live by redundant conditions alone*!*

Fragment N13

static String getParameter(String source, String messageIfNotFound) {
  Matcher matcher = PLACEHOLDER_PARAM_PATTERN.matcher(source);

  while (matcher.find()) {
    return matcher.group(1).replaceAll("'", ""); // <=
  }

  if (messageIfNotFound != null) {
    throw new RuntimeException(messageIfNotFound);
  }

  return null;
}

Warning:

V6037 An unconditional 'return' within a loop.

I have a feeling this while *was raised by *ifs. This code may have some hidden intentions, but the analyzer and I see the same thing here: a loop that always performs one iteration.

The code author might have wanted a different behavioral outcome. Even if they don't want, we'll find this code a bit harder to understand than if there is just a conditional operator.

String comparison

In the following snippet, a developer suggests everything is so easy. But it turns out it's not.

Fragment N14

public boolean equals(Object o) {
  if (this == o) return true;
  if (o == null || getClass() != o.getClass()) return false;

  Key key = (Key) o;

  if (action != key.action) return false;         // <=
  ....
}

Warning:

V6013 Strings 'action' and 'key.action' are compared by reference. Possibly an equality comparison was intended.

Comparing strings implies that we compare their contents. We actually compare object references. This also applies to arrays and collections, not only strings. I think it's clear that in certain cases, code operations can lead to unexpected consequences. Most importantly, it's pretty easy to fix such an error:

public boolean equals(Object o) {
  ....
  if (!action.equals(key.action)) return false;
  ....
}

The equals method returns a comparison exactly to the contents of two strings. Victory!

I'll draw your attention to the fact that the static analyzer has detected the error, which a developer would most likely have missed when reviewing the code. You can read about this and other reasons for using static analysis in this article.

Double-checked locking

Double-checked locking is a parallel design pattern used to reduce the overhead of acquiring a lock. We first check the lock condition without synchronization. If it's encountered, the thread tries to acquire the lock.

If we streamline it, this pattern helps get the lock only when it's actually needed.

I think you've already guessed that there are bugs in the implementation of this template as I've started talking about it. Actually, they are.

Fragment N15

public class WelcomeResource {
  private AtomicBoolean shouldBootstrap;

  ....

  private boolean shouldBootstrap() {
    if (shouldBootstrap == null) {
      synchronized (this) {
        if (shouldBootstrap == null) {
          shouldBootstrap = new AtomicBoolean(....);
        }
      }
    }
    return shouldBootstrap.get();
  }

Warning:

V6082 Unsafe double-checked locking. The field should be declared as volatile.

The analyzer warns that the shouldBootstrap field doesn't have the volatile modifier. What does it affect? In such code, it's likely that different threads use an object until they're fully initialized.

This fact doesn't seem to be that significant, does it? In the next example, the compiler may change the action order with non-volatile fields.

Fragment N16

public class DefaultFreeMarkerProviderFactory 
  implements FreeMarkerProviderFactory {

    private DefaultFreeMarkerProvider provider;   // <=

    ....

    public DefaultFreeMarkerProvider create(KeycloakSession session) {
      if (provider == null) {
        synchronized (this) {
          if (provider == null) {
            if (Config.scope("theme").getBoolean("cacheTemplates", true)) {
              cache = new ConcurrentHashMap<>();
            }
            kcSanitizeMethod = new KeycloakSanitizerMethod();
            provider = new DefaultFreeMarkerProvider(cache, kcSanitizeMethod);
          }
        }
      }
      return provider;
    }
}

Warning:

V6082 Unsafe double-checked locking. The field should be declared as volatile.

如果 bug 如此危險，為什麼開發人員不修復這些程式碼片段？這個錯誤不僅是危險的，而且是偷偷摸摸的。大多數時候一切都以其應有的方式進行。發生不良行為的原因有很多，例如，由於使用了 JVM 或執行緒調度程序操作。因此，重現錯誤條件可能相當困難。

您可以在文章中閱讀有關此類錯誤及其原因的更多資訊。

結論

在本文的最後，我想指出我使用的分析器有點不正常。我只是想娛樂你並向你展示項目中的錯誤。但是，為了修復並防止錯誤，最好在編寫程式碼時定期使用靜態分析器。您可以在這裡閱讀更多相關資訊。

但是，分析器幫助我們發現了與程式運作和程式碼清潔度不足相關的各種錯誤（我仍然認為這很重要，你很難改變我的想法）。如果您及時發現並修復錯誤，那麼錯誤並不是世界末日。靜態分析對此有幫助。試試 PVS-Studio 並用它免費檢查您的專案。

以上是授權陷阱：Keycloak 隱藏了什麼？的詳細內容。更多資訊請關注PHP中文網其他相關文章！