Авторизация и регистрация пользователей — важная часть приложения как для юзеров, так и для безопасности. Но какие неожиданности содержит исходный код популярного Open Source решения для авторизации, и как они могут повлиять на работу приложения?

Все, кто когда-нибудь реализовывал авторизацию для веб-приложений, наверняка смогут вам рассказать множество историй о том, какие проблемные ситуации она создавала в процессе. И я не исключение.

Однажды в проекте мне нужно было реализовать авторизацию через мессенджер во Frontend-части приложения. Задача казалась самой простой на Земле, но в итоге получилось наоборот: сроки горят, код спотыкается об API мессенджера, а люди вокруг начинают кричать.

После этого случая товарищ показал мне хороший инструмент для того, чтобы в будущем мы не испытывали проблем с авторизацией в наших проектах. Этим проектом был Keycloak — Open Source решение для реализации единого входа (Single Sign-On, SSO) и управления доступом к современным приложениям и сервисам, написанный на языке Java.

От того, что я сам пользуюсь этим решением, мне стало ещё интереснее залезть в его исходный код и с помощью статического анализатора PVS-Studio посмотреть на жучков, которые там спрятались.

Классический NullPointerException

В дверь постучали. Штирлиц попытался открыть дверь и сломался. "NullPointerException!" — догадался Мюллер.

Ошибки, связанные с проверкой на null, это тип ошибок, встречающийся почти в каждом проекте, который мы проверяли ранее. Получается, начнём с классики.

Фрагмент 1

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 Potential null dereference of 'rs'.

В этом фрагменте есть проверка на null, но важно то, что в ней происходит. При истинности переменной oscpFailOpen программа не выбросит исключения, а лишь сохранит лог о произошедшем и продолжит выполнение, получив NullPointerException уже в следующем if, поскольку в нём переменная rs уже используется.

Может показаться, что если бы не NullPointerException, то программа бы просто бросила другое исключение, но это не так, поскольку переменная oscpFailOpen является истинной, и программа должна была продолжать выполнение, а в итоге споткнулась об нулевой указатель.

Фрагмент 2

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 The 'attributeValue' reference was utilized before it was verified against null.

"Лучше поздно, чем никогда" — фраза, подходящая к большому количеству ситуаций, но, к сожалению, не к проверке на null. В приведённом выше фрагменте объект attributeValue используется до того, как он будет проверен на существование, что приводит к возникновению NullPointerException.

Примечание. Если вам интересны и другие примеры подобных ошибок, то у нас есть целый список!

Аргументируем...

Не знаю, как так вышло, но в Keycloak ошибки, связанные с количеством аргументов, встретились именно в функциях, форматирующих строки. Не то чтобы репрезентативная статистика, просто интересный факт.

Фрагмент 3

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 Incorrect format. A different number of format items is expected. Arguments not used: 1.

В приведённом фрагменте при вызове функции printf передаётся строка для форматирования и значение, которое в неё необходимо подставить. Только есть одна проблема: в строке просто нет места для подстановки аргументов.

Также интересен тот факт, что из-за полного копирования блока из if в else if эта ошибка тоже была продублирована.

А вот в следующем фрагменте, наоборот, произошёл недобор аргументов...

Фрагмент 4

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

Предупреждение:

V6046 Incorrect format. A different number of format items is expected. Missing arguments: 4.

Помимо переданных в функцию аргументов authSessionId, clientUUID и authNotesFragment также ожидается четвёртый аргумент, названный в строке tabId.

В этом случае метод String.format бросит исключение IllegalFormatException, что может оказаться неприятной неожиданностью.

Нечистый код

Следующий список предупреждений, которые выдал статический анализатор PVS-Studio, не влияет непосредственно на работоспособность кода. Все они скорее касаются того, насколько качественный код был написан. Обращу внимание, что это не ошибки в истинном понимании этого слова.

"Почему же тогда мы будем на них смотреть?" — возможно подумали вы. Во-первых, я считаю, что код должен быть чистым и красивым. И это не вопрос вкусов, ведь помимо визуальных качеств чистота кода влияет на его читаемость, что, как мне кажется, важно для любого проекта, а тем более для Open Source. Во-вторых, мне было интересно показать, что статический анализатор PVS-Studio помогает не только исправлять ошибки в коде, но и наводить в нём красоту и порядок.

Нам нужно больше переменных!

Следующий фрагмент почему-то представляется в моей голове как злобный план человека, плохо относящегося к переменным: "А давайте-ка заведём переменные, а после оставим их с ужасом ожидать, пока до них дойдёт сборщик мусора..."

Фрагмент 5

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

Предупреждение:

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

С точки зрения работы программы в этом фрагменте не произошло ничего страшного. Но неясно, для чего именно это было сделано.

Переменная num содержит количество установленных параметров, которое возвращает метод executeUpdate, поэтому я подумал, что, возможно, в этом методе также была проверка на то, произошли ли какие-то изменения. Но даже отмотав время назад, я обнаружил лишь, что изначально вызовы этого метода не записывались в переменную, а приняли текущее состояние чуть позднее.

Итого получается бесполезное присваивание переменной, как и в следующем фрагменте.

Фрагмент 6

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) {
      ....
    }
}

Предупреждение:

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

Если обратить внимание на код, то видно, что до этого переменной codeVerifierEncoded уже было присвоено то же самое значение, что присваивается в блоке else условного ветвления. То есть просто выполнено лишнее действие, которое мало того, что бесполезно, так ещё и утяжеляет код.

А вот следующий фрагмент кода просто показался мне забавным.

Фрагмент 7

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;
}

Предупреждение:

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

Похоже на предыдущий фрагмент, однако здесь степень моего непонимания необходимости представленного кода принимает на порядок более высокие значения.

Изначально я подумал, что здесь мы имеем дело со вложенным циклом, и автор просто перепутал переменные i и j. Правда, после этого я всё же констатировал, что цикл здесь один.

Другое моё предположение было в том, что такое присваивание появилось в процессе рефакторинга, и что ранее здесь был более сложный код. Но в итоге я обнаружил, что эта функция изначально была создана именно таким образом (коммит).

Любимый Copy-Paste

Упущения при копировании встречаются довольно часто. Думаю, что вы даже прямо сейчас могли вспомнить несколько ситуаций, когда такое встречалось у вас в коде.

В Keycloak тоже остались следы от использования Copy-Paste.

Фрагмент 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");
    ....
  }
  ....
}

Предупреждение:

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

Честно, даже зная о том, что здесь есть опечатка, и какая именно, мне довольно затруднительно было сходу найти её в исходном коде.

Если вы обратите внимание, в вызовах метода schemaLocations.put передаваемые аргументы довольно похожи друг на друга. Поэтому, как я предполагаю, человек, писавший этот код, просто скопировал какую-то строку в качестве образца, а далее менял в ней значения. Проблема в том, что в процессе копирования в проект закралась одна строка, повторяющая предыдущую.

Подобные "очепятки" могут как приводить к серьёзным последствиям, так и ни на что не влиять. Конкретно этот пример Copy-Paste существует в проекте с 21 ноября 2017 года (коммит), и как я думаю, больших трудностей он не создал.

Цель включения этой ошибки в статью исключительно в том, чтобы напомнить, что при копировании нужно быть аккуратнее и следить за тем, что именно меняется. А подробнее прочитать про опечатки, связанные с Copy-Paste, можно в этой статье.

Недостижимый код

Заголовок даёт небольшую подсказку, какого рода предупреждение ждёт нас в следующем фрагменте. Но я предлагаю вам самостоятельно обнаружить недостаток.

Фрагмент 9

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;
    }
}

Полагаю, что конкретно диагностировать проблему не получилось. Я ни капли не злорадствую и дал вам самостоятельно попробовать оценить обстановку на глаз исключительно из-за того, что маленький недостаток этого кода связан с тем, что трудно обнаружить, не зная остального кода. Ответ на то, какая ошибка спрятана в этом фрагменте, даст содержимое метода executeOnAuthorizationRequest:

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

Да, этот метод бросает исключение. То есть весь код, написанный после вызова этого метода, будет недостижимым, что и диагностировал анализатор PVS-Studio.

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;
    }
}

Предупреждение:

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

Пусть этот недочёт и совсем небольшой, но подобная ситуация могла произойти уже с более серьёзными последствиями. Тут лишь могу отметить, что статический анализатор поможет избежать таких неприятных моментов.

Здесь условия диктую я

Теперь заглянем в условные операторы и обратим внимание на то, при каких ситуациях они выполняют свой код.

Фрагмент 10

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))) { 
      ....
    }
}

Предупреждения:

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

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

Сразу два предупреждения в одной строке! На что же тут ругается анализатор? Проблема в том, что самый первый условный оператор проверяет, что пароль не равен null и не пустой, прекращая в обратном случае выполнение функции. А вот в той строке, на которую указал анализатор, обе эти проверки повторяются, то есть условия всегда являются положительными.

С одной стороны, лишними проверки не бывают, а с другой — из-за таких повторений есть вероятность упустить ту часть условия, которая может быть эквивалентна false и действительно влиять на работу программы.

Повторим упражнение.

Фрагмент 11

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

Предупреждение:

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

В целом, идентичная ситуация. Если переменная ssp принимает значение null, то программа бросает исключение. Исходя из этого, условие ниже не только постоянно истинно, так ещё и в принципе не нужно, поскольку соответствующий ему блок кода будет выполняться всегда.

Таким же ненужным является и условие из следующего фрагмента.

Фрагмент 12

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

Предупреждение:

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

В этом методе при разных временах года, фазах луны, и, что самое важное, при разных условиях, выполняется один и тот же код. То есть, опять же, условие здесь было совсем не нужно.

Немного покопавшись в коде, я обнаружил фрагмент, который похож на этот как две капли воды:

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

Как видно, при выполнении условия вызывается метод, который меняет идентификатор сессии.

Исходя из этого, можно сделать два предположения: либо код просто скопировали и поменяли результат условия, либо в первом условии всё же должна была обновляться сессия, и эта ошибка уже выходит за границы нечистого кода.

Но не только лишними условиями едины!

Фрагмент 13

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;
}

Предупреждение:

V6037 An unconditional 'return' within a loop.

Есть ощущение, что этот while воспитали if'ы. Возможно, этот код имеет какой-то скрытый замысел, однако мы с анализатором видим здесь одно и то же: цикл, который всегда выполняет одну итерацию.

Возможно, автор этого кода предполагал другой вариант его поведения. А если даже и нет, то понять смысл этого кода будет чуть сложнее, чем если бы там был просто условный оператор.

Сравнение строк

Автор кода из следующего фрагмента предположил, что всё просто. Но оказалось, что это не так…

Фрагмент 14

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;         // <=
  ....
}

Предупреждение:

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

При сравнении строк подразумевается, что мы сравниваем их содержимое. Однако на самом деле сравниваются ссылки на объекты. То же самое происходит не только со строками, а ещё с массивами и коллекциями. Думаю, понятно, что в определённых ситуациях работа кода может привести к неожиданным последствиям. Самое главное, что исправить такую ошибку довольно легко:

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

Метод equals возвращает сравнение именно к содержанию двух строк. Победа!

А я обращу ваше внимание, что статический анализатор обнаружил ту ошибку, которую разработчик при осмотре кода, скорее всего, пропустил бы дальше. Об этой и других причинах использования статического анализа можно почитать в этой статье.

Блокировка с двойной проверкой

Блокировка с двойной проверкой — это параллельный шаблон проектирования, предназначенный для уменьшения накладных расходов получения блокировки. Сначала мы проверяем условие блокировки без синхронизации, и, если оно выполняется, поток пытается получить блокировку.

Если упростить, то за счёт этого шаблона блокировка будет получаться только тогда, когда она действительно необходима.

Думаю, вы уже догадались о наличии ошибок с реализацией этого шаблона, раз уж я начал о нём рассказывать. Собственно, так и есть.

Фрагмент 15

public class WelcomeResource {
  private AtomicBoolean shouldBootstrap;

  ....

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

Предупреждение:

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

Анализатор ругается, что у поля shouldBootstrap отсутствует модификатор volatile. Но на что это влияет? В таком коде вероятно, что объект может быть использован разными потоками до окончания процесса их инициализации.

И если этот факт может быть не так существенен, то уже в следующем примере компилятор вполне может поменять порядок выполнения действий с не-volatile полями.

Фрагмент 16

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;
    }
}

Предупреждение:

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

Почему тогда эти фрагменты не исправили, если ошибка такая опасная? Ответ в том, что ошибка не только опасная, но ещё и коварная. В большинстве случаев всё работает так, как и должно. А вот неправильное поведение может произойти из-за множества различных факторов, например, используемой JVM или решений планировщика потоков, из-за чего воспроизвести условия возникновения ошибки бывает довольно тяжело.

Подробнее про подобные ошибки и причины их возникновения можно прочитать в статье.

Заключение

В завершение этой статьи хочу отметить, что в ней я использовал анализатор не совсем верно. Моей целью было развлечь вас и показать, какие ошибки есть в проекте. Однако для непосредственного исправления ошибок и профилактики их дальнейшего возникновения статический анализатор нужно использовать не разово, а систематически в процессе написания кода. Об этом подробнее можно прочитать здесь.

Несмотря на это, вместе с анализатором мы обнаружили довольно разнообразный набор ошибок, связанных как с работой программы, так и с недостаточной чистотой кода (всё ещё считаю, что это важно, и вы вряд ли меня переубедите). И ошибки — это не страшно, если они вовремя находятся и исправляются, с чем и поможет статический анализ. А попробовать анализатор PVS-Studio и с его помощью проверить проект можно бесплатно.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Valerii Filatov. Authorization pitfalls: what does Keycloak cloak?