저자: Valerii Filatov
사용자 승인 및 등록은 사용자뿐만 아니라 보안을 위해서도 모든 애플리케이션에서 중요한 부분입니다. 인기 있는 오픈 소스 ID 관리 솔루션의 소스 코드에는 어떤 함정이 숨겨져 있습니까? 애플리케이션에 어떤 영향을 미치나요?
웹 앱에 대한 인증을 구현해 본 적이 있다면 발생할 수 있는 불편한 문제를 모두 알고 계실 것입니다. 나도 예외는 아니다.
한번은 한 프로젝트의 프론트엔드 부분에 메신저 기반 인증을 구현한 적이 있습니다. 세상에서 가장 쉬운 일처럼 보였지만 결과는 정반대였습니다. 마감 시간이 다가오고, 코드가 메신저 API에 걸려 넘어지고, 주변 사람들이 소리를 지르고 있었습니다.
이 사건 이후 제 동료는 향후 프로젝트에서 인증 구현을 간소화할 수 있는 멋진 도구를 보여주었습니다. 이 프로젝트는 최신 앱과 서비스를 목표로 하는 ID 및 액세스 관리를 통해 SSO(Single Sign-On)를 지원하는 오픈 소스 Java 솔루션인 Keycloak이었습니다.
솔루션을 직접 사용하다보니 소스코드에 들어가서 PVS-Studio 정적 분석기를 사용해 거기 숨어있는 버그를 찾아보는 재미가 쏠쏠합니다.
누군가 문을 두드렸습니다. Junior Dev가 문을 열려고 하다가 추락했습니다. "널포인트 예외!" 수석개발을 마쳤습니다.
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 변수가 rs 변수에서 이미 사용되었기 때문에 다음 if에서 NullPointerException을 수신합니다. 🎜>만약
.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에 대해 검증되기 전에 활용되었습니다.
"Better late than never"라는 문구는 많은 경우에 충분하지만 안타깝게도 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.
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).
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.
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.
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.
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 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 중국어 웹사이트의 기타 관련 기사를 참조하세요!
![[웹 프런트엔드] Node.js 빠른 시작](https://img.php.cn/upload/course/000/000/067/662b5d34ba7c0227.png)
