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

作者：瓦莱里·菲拉托夫

用户授权和注册是任何应用程序的重要组成部分，不仅是为了用户，也是为了安全。流行的开源身份管理解决方案的源代码隐藏了哪些陷阱？它们如何影响应用程序？

如果您曾经为网络应用程序实现过授权，您可能知道可能出现的所有令人沮丧的问题。我也不例外

有一次，我在一个项目的前端部分实现了基于Messenger的授权。这似乎是世界上最简单的任务，但事实却恰恰相反：最后期限迫在眉睫，代码被消息 API 绊倒，我周围的人都在大喊大叫。

在这个案例之后，我的同事向我展示了一个很酷的工具，可以简化我们未来项目中授权的实施。这个项目是 Keycloak，一个开源 Java 解决方案，可实现单点登录 (SSO) 以及针对现代应用程序和服务的身份和访问管理。

当我自己使用该解决方案时，我发现进入源代码并使用 PVS-Studio 静态分析器来查找隐藏在那里的错误很有趣。

呵呵，经典的NullPointerException

有人敲门。初级开发人员试图打开一扇门，结果摔坏了。 “空指针异常！”高级开发人员总结

与检查null相关的错误在我们之前检查过的几乎每个项目中都会遇到。那么，让我们从这个古老但黄金的错误开始。

片段N1

警告：

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

此代码片段有一个null检查，但我们需要了解内部发生了什么。如果_oscpFailOpen变量为 true，程序不会抛出异常。它只会保存有关它的日志并继续执行 - 它在下面的if中收到NullPointerException，因为rs变量已经在if.

如果没有NullPointerException，程序可能会抛出另一个异常。但事实并非如此，因为_oscpFailOpen变量为 true，程序应该继续执行。没缘分，被null指针绊倒，陷入了异常。

片段N2

警告*：*

V6060 “attributeValue”引用在验证为空之前已被使用。

“迟到总比不到好”，这句话对于很多情况来说已经足够了，但不幸的是，不适用于null检查。在上面的代码片段中，attributeValue对象在检查其是否存在之前被使用，这导致NullPointerException.

注意。如果您想查看此类错误的其他示例，我们已为您整理了一个列表！

论据？

我不知道这是怎么发生的，但 Keycloak 存在与格式字符串函数中的参数数量相关的错误。这不是一个说明性的统计数据，只是一个有趣的事实。

片段N3

警告：

V6046 格式不正确。预计会有不同数量的格式项。未使用的参数：1.

在上面的片段中，当调用printf函数时，我们传递一个格式字符串和一个要替换到该字符串中的值。只有一个问题：字符串中根本没有地方可以替换参数。

非常有趣的是，开发人员不仅将if的代码片段复制并粘贴到else if，而且还将错误复制并粘贴到else if。

在下一个片段中，我们有相反的情况：开发人员没有争论。

片段N4

警告：

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.

Thenumvariable contains the number of set parameters that theexecuteUpdatemethod 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, thecodeVerifierEncodedvariable has already assigned the same value in theelsebranch. A developer just did redundant action: and useless, and overcomplicating.

The next code fragment just amuses me.

Fragment N7

private static Type[] extractTypeVariables(Map 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 variablesiandj. 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 theschemaLocations.putmethod 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 theexecuteOnAuthorizationRequestmethod:

private void executeOnAuthorizationRequest(MultivaluedMap 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 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-nulland 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 tofalse—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 thesspvariable isnull, 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 thiswhile *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; .... }

Theequalsmethod 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 theshouldBootstrapfield doesn't have thevolatilemodifier. 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-volatilefields.

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.

Why don't developers fix these code fragments if the bug is so dangerous? The error is not only dangerous but also sneaky. Everything works as it should most of the time. There are lots of different reasons why bad behavior can occur, due to, let's say, used JVM or the thread scheduler operations. So, it can be quite hard to reproduce the error conditions.

You can read more about such errors and their reasons in the article.

Conclusion

At the end of this article, I'd like to point out that I used the analyzer a little out of order. I just wanted to entertain you and show you the bugs in the project. However, to fix errors and prevent them, it's better to use the static analyzer regularly while writing code. You can read more about it here.

However, the analyzer helped us spot various errors related both to the program operation and insufficient code cleanliness (I still think it's important, and you'll hardly change my mind). Errors are not the end of the world if you spot and fix them in time. Static analysis helps with this. Try PVS-Studio and use it to check your project for free.

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