認可の落とし穴: Keycloak は何を隠蔽しますか?

著者: ヴァレリー・フィラトフ

ユーザーの承認と登録は、ユーザーだけでなくセキュリティにとっても、アプリケーションの重要な部分です。人気のオープンソース ID 管理ソリューションのソース コードにはどのような落とし穴が隠されていますか?それらはアプリケーションにどのような影響を与えますか?

Web アプリの認証を実装したことがあるなら、おそらく、発生する可能性のあるイライラする問題をすべて知っているでしょう。私も例外ではありません。

かつて、私はあるプロジェクトのフロントエンド部分にメッセンジャーベースの認可を実装しました。それは世界で最も簡単なタスクのように思えましたが、実際はその逆でした。締め切りは迫っており、コードはメッセンジャー API でつまずき、周りの人たちは怒鳴っていました。

このケースの後、私の同僚は、将来のプロジェクトでの認可の実装を効率化する素晴らしいツールを私に見せてくれました。このプロジェクトは Keycloak で、最新のアプリやサービスを対象とした ID およびアクセス管理によるシングル サインオン (SSO) を可能にするオープンソース Java ソリューションです。

私自身このソリューションを使用しているので、ソース コードに入り込み、PVS-Studio 静的アナライザーを使用してそこに隠されたバグを探すのが興味深いと思います。

へー、古典的な NullPointerException

誰かがドアをノックしました。ジュニア開発者はドアを開けようとして衝突しました。 「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 逆参照。

このコード スニペットには null チェックがありますが、内部で何が起こっているのかを理解する必要があります。 _oscpFailOpen 変数が true の場合、プログラムは例外をスローしません。それに関するログのみを保存して実行を続行します。rs 変数が < 内ですでに使用されているため、次の if で NullPointerException を受け取ります。 🎜>if.

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 に対して検証される前に利用されました。

「遅刻しないよりはマシ」というフレーズは、多くの場合に十分に適していますが、残念ながら、

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

関数が呼び出されるときに、書式文字列と文字列に置換される値を渡します。問題は 1 つだけあります。文字列内に引数を置き換える場所がないということです。 非常に興味深いのは、開発者が

if

から else if までの両方のコード フラグメントをコピーして貼り付けただけでなく、else 内のエラーもコピーして貼り付けたことです。 if. 次のスニペットでは、逆のケースが示されています。開発者は引数を省略しています。

フラグメント N4

警告:

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

V6046 形式が正しくありません。異なる数のフォーマット項目が予期されます。引数が欠落しています: 4.

開発者は

authSessionId

、clientUUID、および authNotesFragment 引数を関数に渡しましたが、4 番目の 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.

バグがそれほど危険であるのに、なぜ開発者はこれらのコードの断片を修正しないのでしょうか?このエラーは危険であるだけでなく、卑劣でもあります。ほとんどの場合、すべてが正常に機能します。不正な動作が発生する理由はさまざまですが、たとえば、使用された JVM やスレッド スケジューラの操作が原因です。したがって、エラー状態を再現するのは非常に難しい場合があります。

このようなエラーとその理由について詳しくは、記事をご覧ください。

結論

この記事の最後に、アナライザーの使用方法が少し間違っていたことを指摘しておきます。私はただ皆さんを楽しませて、プロジェクトのバグを見せたかっただけです。ただし、エラーを修正して防止するには、コードの作成中に静的アナライザーを定期的に使用することをお勧めします。詳細については、こちらをご覧ください。

しかし、アナライザーは、プログラムの動作と不十分なコードのクリーンさの両方に関連するさまざまなエラーを発見するのに役立ちました (私は今でもそれが重要だと考えています、そしてあなたの考えはほとんど変わらないでしょう)。エラーを見つけて時間内に修正できれば、エラーは世界の終わりではありません。静的分析はこれに役立ちます。 PVS-Studio を試して、プロジェクトを無料でチェックしてください。

以上が認可の落とし穴: Keycloak は何を隠蔽しますか?の詳細内容です。詳細については、PHP 中国語 Web サイトの他の関連記事を参照してください。