Comments 31
Diff-viewer, который не показывает всю строку? Выглядит, как фантастика
Выглядит, как фантастика, но легко воспроизводится
Пример программы/веб-сервиса приведете? Хотя бы скриншот?
Вот полный пример: https://github.com/fimskiy/evil-merge-demo
В PR виден только "superuser", но в итоговом файле в main есть оба токена — "hacked" и "superuser". Можешь сам посмотреть merge-коммит и убедиться.
Я не спорю с тем, что создать такую ситуацию возможно, это же штатный механизм разрешения конфликтов. Но вы утверждаете
туда, куда ни один diff-вьюер не прокрутит и ни один редактор не покажет без горизонтального скролла.
что при ревью этого не видно поскольку вам программа не показывает хвост где-то там за границами экрана и полосы прокрутки тоже нет. Вот к этому у меня большие вопросы – что это за программа такая, которая так делает. К слову, GitHub делает переносы строк, поэтому никаких секретных данных вы за границами экрана не спрячете.
В статье я допустил неточную формулировку про скролл. Суть проблемы не в этом. Дело в том, что GitHub не показывает вообще строку в PR-diff. Это не про длинные строки, а про то, что изменения, добавленные в самом merge-коммите (поверх результата слияния), не отображаются в PR.
GitHub показывает diff: feature branch vs main.
GitHub не показывает: что было добавлено в merge-коммите после слияния.
Спасибо за уточнение, я обновил формулировку в статье.
Поэтому я всегда включаю режим soft-wrap в IDE)
Вроде не перевод, а как будто перевод.
Я, все таки, не доктор, но разве разрешение конфликтов не делает именно это? Меняет файлы в процессе мёрджа.
Или это вопрос исключительно к утверждению MR/PR в гитхаб/гитлаб?
Я так понял весь смысл что именно во время разрешения конфликта есть возможность внедрить вредонос и обычно ревьювер уже дал свой аппрув типа: "Всё ок, вот аппрув, только разреши конфликты и вольём" и вот тут всплывает проблема что ревьюверу больше не требуется снова делать ревью (решение что дала поддержка GitHub заставит ревьювера снова смотреть diff и давать аппрув)
Именно так. Классический сценарий: ревьюер одобрил PR, попросил только разрешить конфликты — и после этого повторного ревью не требуется. Атакующий в момент разрешения конфликта добавляет произвольный код в merge-коммит, и этот код уходит в main без чьего-либо взгляда.
▎ Что интересно — решение, которое GitHub упомянул как возможное направление (“may make this functionality more strict”), как раз и означало бы: после изменений в merge-коммите аппрув сбрасывается и требуется повторное ревью. Пока этого нет,
единственная защита — инструментальная проверка самого merge-коммита.
При разрешении конфликта все изменения видны в diff PR’а — ревьюер может их проверить. Evil merge — это изменения, которых нет ни в одной из веток-родителей и которые не являются конфликтом. Они добавляются напрямую в merge-коммит после того, как git уже сделал слияние. GitHub в таком случае показывает в PR только diff относительно base branch — и эти изменения в нём не видны совсем.
Меня в своё время посадили на rebase + fast forward ( + cherry pick при крайней необходимости). Merge коммиты запрещены на уровне репозитория. Правда делалось это изначально с другой целью - иметь чистую историю изменений, которая 1-в-1 берётся в качестве changelog при релизах.
Rebase + fast-forward полностью закрывает эту атаку — merge-коммитов нет, вектора нет. Чистая история как бонус. Если команда может себе это позволить, это лучшее решение. Проблема в том, что большинство крупных проектов используют merge-коммиты — GitHub создаёт их по умолчанию, и далеко не все меняют эту настройку. Для них и нужен инструментальный контроль.
Эээ разве эта ситуация не решается запретом коммитить в мастер-ветку? (в результате чего merge-коммит делается автоматически, без возможности что-то туда добавить)
Да, это усложняет атаку в GitHub, но полностью проблему не убирает. При достаточных правах правила можно обойти. И это не защищает от уже существующих изменений в истории.
Так а если есть права для обхода запрета - разве это само по себе не дает злоумышленнику запушить какой угодно зловред? Вообще без мерж коммитов.
Почему проблема именно с мерж коммитами?
Проблема именно с merge-коммитами, потому что в GitHub UI можно редактировать конфликты прямо в браузере, добавляя extra code. При rebase + fast-forward конфликты разрешаются локально, и такой фокус не пройдёт.
В вашем репозитории (https://github.com/fimskiy/evil-merge-demo) вы сделали именно так? Редактировали конфликт в UI гитхаба при мердже?
Нет, в этом примере evil merge сделан локально через git commit --amend — это более простой способ для демонстрации.
Через GitHub UI сценарий немного другой: при наличии конфликта GitHub предлагает редактировать файл прямо в браузере, и в этот момент можно добавить extra code. Конечный эффект тот же — в merge-коммит попадает код, которого не было в PR.
Так есть локально вы меняли мердж коммит уже мастере? И пушили тоже в мастер напрямую? Если так, то да, неубедительно пополучается.
Хотелось бы демо, где в мастер пушить запрещено, а зловред пролезает.
Я тоже согласен что пример надо сделать тот, что описывается в статье, иначе многим непонятно
Сделал демо с branch protection: https://github.com/fimskiy/evil-merge-demo/pull/9
Сценарий:
Branch protection включён — require 1 approving review
Второй аккаунт (reviewer) одобрил PR, видел только "superuser"
Мейнтейнер смержил локально, добавил "hacked" token в merge-коммит
Запушил в main через admin bypass
Ключевой момент: в момент апрува merge-коммита ещё не существует — он создаётся только при нажатии “Merge”. Ревьюер физически не может его видеть, потому что его ещё нет. После merge ссылка на коммит 92a08fd появляется внизу PR — но туда нужно специально идти и знать что искать.
Да, если после merge специально кликнуть на hash коммита — evil code виден. Атака не про техническую невозможность обнаружения, а про то, что очень часто никто этого не делает в рутинной работе. Именно так это работало 3,5 месяца в реальном проекте с CI, code review и несколькими разработчиками.
4 Запушил в main через admin bypass
А зачем тогда все остальное? Злоумышленник с admin bypass доступом может просто лить что угодно в мастер без всяких мерджей и ПР. Нет ПР - нет ревью. Эффект тот же: "непроревьюенный зловред в мастере".
Возможно, поэтому команда гитхаба и отмахнулась от репорта? Принципиально нового вектора атаки-то нет.
Так ничего не мешает в свою ветку намешать левых мерж коммитов из других веток. И если вкоммичтвание мерж коммитов явно не запрещено.
Код ревью мешает
Вы часто ревьювите именно мерж коммиты? Особенно если это вливание больших фичей. Статья ведь как раз об этом.
Я ревьюю не коммиты, а ПР в целом.
Если из-за здоровенных мерж коммитов из левых веток в ПР 100500 измененных строчек - это мало чем отличается от просто создания ветки с 100500 изменений, в которых я где-то спрятал бэкдор.
В статье речь исключительно про изменения, вносимые после ревью кода. Предполагается что ревью было тщательное и в вдиффе действительно ничего криминального нет
Во многих тулзах для ревью по умолчанию отключен показ содержимого мерж коммитов. Я считаю это неправильным, но увы, «сверху» боятся ужесточать воркфлоу.
Реальная ситуация: делаем отдельный бранч А для большой фичи. В процессе разработки другая команда подготовила бранч Б с фичами которые нам нужны, но которые нельзя лить в мастер. Поэтому мы мержим себе бранч Б после код ревью. Девелопер, который нажимал кнопку «мерж» оказался не чистый на руку и в мерж коммит «Б в А» добавил вредоносный код. (Да да, человеческий фактор, который можно обойти качественным git workflow, либо утилитами наподобие той что в статье). Далее, бранч А готов к мержу в Мастер. И вот для этого код ревью не будет показан мерж коммит Б в А.
Самый простой способ этого избежать - отказ от мерж коммитов и работа исключительно через rebase+squash+cherry pick. Надеюсь, теперь вам моя мысль ясна :)
Evil Merge: как малварь пряталась в git merge-коммите 3,5 месяца