28 KiB
Процесс 9: Экспертиза исходного кода
Дата публикации: 23 марта
Время на чтение: ~12 мин
ТГ канал: Про_SEC_со
Merge Request на 1200 строк. Пятница, пять вечера. Ревьюер пробегает глазами diff и нажимает Approve. В понедельник этот код уже в продакшене. А через две недели безопасник на пентесте находит в нём обход авторизации: один эндпоинт отдаёт данные без проверки роли пользователя. Вся цепочка сработала "правильно" –– CI прошёл, тесты зелёные, ревью одобрено. Только вот ревью было формальным, и логическую ошибку никто не увидел.
Именно поэтому ГОСТ Р 56939-2024 выделяет экспертизу исходного кода в отдельный процесс (5.9), а не растворяет её в "ну, у нас есть code review в GitLab". Процесс 9 –– про то, чтобы человеческий анализ кода был системным: с регламентом, ролями, чек-листом и фиксацией результатов. Не "кто-то посмотрел", а конкретный специалист проверил конкретные области по конкретным критериям.
Суть процесса простыми словами
Экспертиза исходного кода –– это анализ кода человеком, который не писал этот код. Речь не про быстрый approval в MR, а про осмысленную проверку по заранее определённым критериям.
Важно не путать процесс 9 с процессом 10 (статический анализ). Разница принципиальная:
- Процесс 10 –– автопроверка. SAST-инструмент (Semgrep, SonarQube, Checkmarx, Bandit) автоматически ищет паттерны: SQL-инъекции, захардкоженные секреты, use-after-free. Автоматическая проверка быстра, но слепа к логике –– она не знает, что этот эндпоинт должен быть доступен только админу.
- Процесс 9 -- человек. Ревьюер понимает контекст: зачем написан код, какие требования безопасности к нему предъявлены, как он ложится в архитектуру, нет ли обхода бизнес-логики. Человек медленнее, но видит то, чего инструмент не может.
ГОСТ требует оба процесса. Инструмент ловит типовые дефекты в масштабе, человек ловит логические ошибки и подтверждает или дополняет результаты SAST. Вместе они закрывают то, что поодиночке не закроет ни один из подходов.
Ещё одна деталь из примечания ГОСТа: экспертиза особенно важна для кода, к которому нет инструментов статического анализа -- нишевые языки, внутренние DSL, сложные конфигурации. Там ревьюер -- единственный фильтр.
Требование 5.9.2.1: Регламент экспертизы
Разработать регламент проведения экспертизы исходного кода ПО.
Регламент –– это не ещё один документ "для папки". Это договорённость команды: кто, что, когда и как проверяет. Без неё ревью либо не происходит, либо происходит по-разному у каждого –– один смотрит стиль, другой безопасность, третий просто ставит галку.
Артефакт 5.9.3.1: что должно быть в регламенте
ГОСТ требует три блока:
- Роли и обязанности –– кто проводит экспертизу и за что отвечает
- Базовые требования –– сколько ревьюеров, какие области кода подлежат экспертизе, какие инструменты используются
- Описание проверок –– сценарии, шаблоны, чек-листы, по которым проводится экспертиза
Звучит абстрактно? Давайте сделаем конкретно
Пример: фрагмент регламента экспертизы
Роли и обязанности
| Роль | Кто | Обязанности |
|---|---|---|
| Автор | Разработчик, написавший код | Создаёт MR, описывает изменения и контекст, отвечает на вопросы ревьюера, вносит правки |
| Ревьюер | Разработчик, не участвовавший в написании кода | Проверяет код по чек-листу, оставляет комментарии с категорией (blocker / warning / suggestion), принимает или отклоняет MR |
| Security Champion | Назначенный специалист по безопасности в команде | Обязательный ревьюер для кода на поверхности атаки (auth, API, обработка ввода, криптография); проверяет выполнение требований безопасности |
Базовые требования
1. Минимальное количество ревьюеров:
- Общий код: 1 ревьюер
- Код на поверхности атаки (auth, API-gateway, обработка
пользовательского ввода, криптография, работа с секретами):
2 ревьюера, один из которых –– Security Champion.
2. Области кода, подлежащие обязательной экспертизе:
- Любые изменения, попадающие в защищённую ветку (release/*)
- Изменения в модулях из перечня поверхности атаки (см. модель угроз)
- Изменения конфигурации CI/CD пайплайнов
- Изменения зависимостей (package.json, requirements.txt, go.mod и т.д.)
3. Максимальный размер MR для одного ревью: до 400 строк кода.
MR свыше 400 строк рекомендуется разбивать на части.
Если разбить невозможно –– автор обязан предоставить описание
архитектуры изменений и провести walk-through с ревьюером.
4. Срок проведения экспертизы:
- Обычный MR: до 1 рабочего дня
- MR с изменениями на поверхности атаки: до 2 рабочих дней
- Hotfix (критичный баг/уязвимость): в течение 4 часов,
с обязательным post-review в течение 2 рабочих дней
Чек-лист экспертизы безопасности
Чек-лист –– не произвольный список "чего бы проверить". Каждый его пункт прослеживается до конкретного источника:
- Требования безопасности к ПО (процесс 5.3). Если в наборе требований записано "ПО должно реализовать разграничение доступа к API на основе ролей" –– в чек-листе появляется пункт
[ ] Роли и права проверяются на серверной стороне. Требование задаёт что должно быть; чек-лист превращает это в вопрос, который ревьюер проверяет на уровне кода. - Правила кодирования (процесс 5.8). Запрет конкатенации в SQL-запросах, запрет хранения секретов в коде, обязательное экранирование вывода –– всё это идёт из регламента безопасного кодирования.
- Модель угроз и поверхность атаки (процесс 5.7). Категории чек-листа (аутентификация, обработка ввода, криптография) отражают области, где угрозы наиболее вероятны. Если модель угроз обновилась –– чек-лист обновляется следом.
- Инциденты и результаты тестирования (процессы 5.18--5.19, 5.23--5.24). Нашли уязвимость на пентесте или получили отчёт об инциденте –– добавляется пункт, чтобы подобное не повторилось.
По сути, чек-лист –– это проекция требований безопасности, правил кодирования и модели угроз на уровень конкретного diff в MR. Он не заменяет ни один из этих документов, но делает их проверяемыми в ежедневной работе ревьюера.
Этот чек-лист ревьюер проходит при каждом MR, затрагивающем поверхность атаки. Для обычных MR достаточно пунктов, отмеченных [*]:
Чек-лист экспертизы безопасности (Security Code Review Checklist)
Аутентификация и авторизация:
[*] Все эндпоинты проверяют авторизацию –– нет "открытых" путей без явного разрешения
[ ] Роли и права проверяются на серверной стороне, а не только на клиенте
[ ] Токены/сессии имеют ограниченное время жизни и механизм отзыва
Обработка ввода:
[*] Пользовательский ввод валидируется и санитизируется до использования
[*] SQL/NoSQL-запросы используют параметризацию или ORM, не конкатенацию
[ ] Данные для HTML-вывода экранируются или используется textContent/санитизация
Секреты и конфиденциальные данные:
[*] В коде и коммитах нет паролей, ключей, токенов в открытом виде
[*] В логи не попадают токены, пароли, заголовки Authorization/Cookie, ПДн
[ ] Секреты загружаются из переменных окружения или системы управления секретами
Криптография:
[ ] Используются актуальные алгоритмы (не MD5/SHA1 для хеширования паролей)
[ ] Ключи и сертификаты не хранятся в репозитории
Обработка ошибок:
[*] Ошибки не раскрывают внутреннюю структуру системы (стектрейсы, пути, версии)
[ ] Есть обработка неожиданных состояний (таймауты, недоступность зависимостей)
Логика и архитектура:
[ ] Нет обходных путей, все ветки бизнес-логики учитывают требования безопасности
[ ] Race conditions учтены при работе с общими ресурсами
[ ] Нет избыточных привилегий (код не запрашивает больше прав, чем необходимо)
Это не "ещё одна бюрократия" –– это список вопросов, которые ревьюер проходит за 5-10 минут. Без него каждый проверяет "что помнит"; с ним, покрывается минимальный набор типовых проблем. Чек-лист живой: его обновляют после инцидентов, по результатам пентестов и по мере развития продукта.
Требование 5.9.2.2: Проведение экспертизы
Проводить экспертизу определённых областей кода ПО (в первую очередь для модулей (компонентов) ПО, составляющих поверхность атаки) в соответствии с регламентом проведения экспертизы исходного кода ПО.
Две ключевые мысли здесь. Первая: экспертиза проводится в соответствии с регламентом, то есть по тому документу, который вы создали в предыдущем шаге. Вторая: приоритет –– поверхность атаки. Не нужно одинаково глубоко ревьюить правку опечатки в README и новый эндпоинт аутентификации. ГОСТ явно говорит: сначала то, что на линии огня.
Артефакт 5.9.3.2: результаты экспертизы
Результаты должны содержать:
- Проанализированные модули –– что именно проверялось
- Перечень необходимых изменений –– что нужно исправить
- Вопросы к коду –– что непонятно и требует разъяснений от автора
- Предложения по улучшению –– что можно сделать лучше, но не блокирует
Если вы используете GitLab –– MR с комментариями ревьюера и итоговым решением (Approved / Changes Requested) и есть артефакт. Только важно, чтобы комментарии покрывали все четыре пункта, а не были набором "поправь отступ".
Пример: MR-review сервиса авторизации
Вот как может выглядеть реальная экспертиза. Команда разрабатывает REST API на Python/FastAPI. Разработчик создал MR: "Добавить эндпоинт сброса пароля"
Контекст MR:
MR #347: feat: password reset endpoint
Автор: @ivanov
Ревьюеры: @petrov (dev), @sidorova (Security Champion)
Модуль: auth-service (поверхность атаки)
Изменённые файлы:
- auth_service/routes/password_reset.py (+142)
- auth_service/services/token_service.py (+38)
- auth_service/templates/email_reset.html (+27)
- tests/test_password_reset.py (+95)
Комментарии ревьюера (категоризация: blocker / warning / suggestion / question):
--- password_reset.py, строка 34 ---
[BLOCKER] @petrov:
Токен сброса генерируется через random.randint(). Это предсказуемо,
атакующий может перебрать пространство токенов.
Использовать secrets.token_urlsafe(32).
--- password_reset.py, строка 58 ---
[BLOCKER] @sidorova:
Эндпоинт POST /reset-password/{token} не ограничивает количество попыток.
Даже с криптостойким токеном отсутствие rate limiting –– риск.
Добавить rate limiter (например, slowapi): не более 5 попыток
в минуту на IP, не более 3 запросов на сброс по одному email в час.
--- password_reset.py, строка 71 ---
[WARNING] @sidorova:
При несуществующем email эндпоинт возвращает 404 с сообщением
"User not found". Это раскрывает факт наличия/отсутствия аккаунта
(user enumeration). Для любого ввода возвращать 200 с одинаковым
текстом: "Если учётная запись существует, письмо отправлено".
--- token_service.py, строка 12 ---
[WARNING] @petrov:
Время жизни токена 24 часа. Для сброса пароля это слишком много.
Рекомендация: 30 минут, максимум 1 час. После использования,
инвалидировать токен немедленно.
--- password_reset.py, строка 89 ---
[QUESTION] @petrov:
После успешной смены пароля вы инвалидируете только текущий токен
сброса. А действующие сессии пользователя? Нужно ли отзывать все
активные сессии при смене пароля? Если да, то добавить вызов
session_service.revoke_all(user_id).
--- email_reset.html, строка 15 ---
[SUGGESTION] @sidorova:
В шаблоне письма ссылка формируется через конкатенацию:
href="{{ base_url }}/reset?token={{ token }}"
Лучше явно экранировать token через |urlencode на случай, если
логика генерации токенов когда-то изменится.
--- test_password_reset.py ---
[SUGGESTION] @petrov:
Тесты покрывают happy path, но нет негативных кейсов:
- Попытка сброса с истёкшим токеном
- Попытка повторного использования токена
- Попытка сброса с невалидным форматом токена
Добавить хотя бы эти три сценария.
Итоговое решение:
Статус: Changes Requested
Блокеры (обязательно исправить до повторного ревью):
1. Заменить random.randint() на secrets.token_urlsafe(32)
2. Добавить rate limiting на эндпоинт сброса
Предупреждения (исправить до мержа):
3. Убрать user enumeration, одинаковый ответ для любого email
4. Сократить TTL токена до 30-60 минут
Вопросы (требуют ответа автора):
5. Инвалидация активных сессий при смене пароля, нужна или нет?
Предложения (желательно, не блокируют):
6. Экранирование токена в шаблоне письма
7. Негативные тест-кейсы
Вот это артефакт 5.9.3.2 в действии. Видны проанализированные модули, конкретные замечания с привязкой к коду, вопросы к автору и предложения по улучшению. Всё зафиксировано в системе контроля версий, ничего не потерялось.
Обратите внимание: два блокера –– это не стилевые придирки, а реальные уязвимости, которые SAST мог не поймать (отсутствие rate limiting, user enumeration). Именно в этом ценность человеческой экспертизы.
Рекомендация 5.9.2.3: Автоматизация экспертизы
Проводить экспертизу кода рекомендуется с использованием программных средств, автоматизирующих проведение экспертизы и интегрированных с системой контроля версий разрабатываемого ПО.
Это рекомендация, не жёсткое требование. Но на практике без автоматизации регламент быстро разваливается: MR мержат без ревью, Security Champion забывают добавить, чек-лист не проходят.
Пример: настройка автоматизации
CODEOWNERS (GitLab/GitHub)
Файл в корне репозитория, который автоматически назначает ревьюеров по путям:
# Файл CODEOWNERS
# Модули на поверхности атаки –– обязательный ревью Security Champion
auth_service/ @security-champions
api_gateway/ @security-champions
payment/ @security-champions
crypto/ @security-champions
# CI/CD конфигурация –– ревью DevOps + Security
.gitlab-ci.yml @devops-team @security-champions
Dockerfile* @devops-team @security-champions
# Зависимости –– ревью тимлида
requirements*.txt @team-leads
package*.json @team-leads
go.mod @team-leads
Branch Protection Rules
Защищённые ветки: main, release/*
Правила:
- MR обязателен для любого изменения (запрет прямых push)
- Минимум 1 approval от ревьюера
- Для путей из CODEOWNERS, approval от указанных владельцев
- CI pipeline должен пройти успешно
- Все threads/discussions должны быть resolved
- Запрет self-approve (автор не может одобрить свой MR)
MR-шаблон с чек-листом
Шаблон, который автоматически добавляется при создании MR и напоминает автору подготовить контекст, а ревьюеру –– пройти проверки:
## Описание изменений
<!-- Что изменено и зачем -->
## Затронутые модули
<!-- Перечислите модули; отметьте, если модуль на поверхности атаки -->
## Чек-лист автора
- [ ] Код соответствует регламенту оформления (процесс 8)
- [ ] Добавлены/обновлены тесты
- [ ] Нет секретов в коде и коммитах
- [ ] Описание MR содержит контекст для ревьюера
## Чек-лист ревьюера (безопасность)
- [ ] Авторизация: все эндпоинты проверяют права доступа
- [ ] Ввод: пользовательские данные валидируются до использования
- [ ] Секреты: в коде и логах нет конфиденциальных данных
- [ ] Ошибки: не раскрывают внутреннюю структуру системы
- [ ] Логика: нет обходных путей бизнес-правил
Интеграция с трекером задач
Каждый MR привязывается к задаче в трекере (Jira, Yandex Tracker, GitLab/Gitflic Issues). Блокеры из ревью автоматически создаются как подзадачи. Это даёт прослеживаемость: по любой находке можно отследить –– была ли она исправлена, кем и когда.
Как это работает вместе
Три требования процесса 9 –– это не три отдельных действия, а одна цепочка.
Регламент (5.9.2.1) задаёт правила игры. Проведение экспертизы (5.9.2.2) -- это игра по этим правилам, с фиксацией результатов. Автоматизация (5.9.2.3) –– инфраструктура, которая не даёт эти правила забыть или обойти.
И процесс 9 не живёт в вакууме. Он опирается на процесс 8 (правила кодирования): ревьюер проверяет код в том числе на соответствие регламенту оформления и безопасного кодирования. И он дополняет процесс 10 (статический анализ): SAST ловит паттерны автоматически, а человек на ревью подтверждает результаты, ловит то, что инструмент пропустил, и оценивает логику.
Вместе эти три процесса –– правила, человеческий анализ, машинный анализ -- дают разумную уверенность, что код, попадающий в продакшен, проверен и по форме, и по сути. Без единого "LGTM на бегу".