From 4cfe8ca73d64cd5c8fdc812c9b3d2967c632c8c4 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Sat, 24 Jul 2021 14:20:06 +0200 Subject: [PATCH] Sanitize user controlled arguments in bot comments (#732) * Sanitize user controlled arguments in bot comments * Make CommandModule ignore comments written by bot --- .../arisa/infrastructure/HelperMessages.kt | 3 +- .../arisa/infrastructure/jira/Functions.kt | 7 +++ .../mojira/arisa/modules/AttachmentModule.kt | 9 ++-- .../mojira/arisa/modules/CommandModule.kt | 15 ++++--- .../commands/ListUserActivityCommand.kt | 10 +++-- .../modules/commands/RemoveContentCommand.kt | 5 ++- .../infrastructure/HelperMessagesTest.kt | 16 +++++++ .../arisa/modules/AttachmentModuleTest.kt | 20 +++++++-- .../mojira/arisa/modules/CommandModuleTest.kt | 23 ++++++++-- .../commands/ListUserActivityCommandTest.kt | 44 ++++++++++++++++--- .../commands/RemoveContentCommandTest.kt | 23 ++++++---- .../github/mojira/arisa/utils/MockDomain.kt | 2 +- 12 files changed, 141 insertions(+), 36 deletions(-) diff --git a/src/main/kotlin/io/github/mojira/arisa/infrastructure/HelperMessages.kt b/src/main/kotlin/io/github/mojira/arisa/infrastructure/HelperMessages.kt index d32788b7e..8ac16ed01 100644 --- a/src/main/kotlin/io/github/mojira/arisa/infrastructure/HelperMessages.kt +++ b/src/main/kotlin/io/github/mojira/arisa/infrastructure/HelperMessages.kt @@ -6,6 +6,7 @@ import arrow.core.right import arrow.core.rightIfNotNull import com.beust.klaxon.Klaxon import com.beust.klaxon.KlaxonException +import io.github.mojira.arisa.infrastructure.jira.sanitizeCommentArg import io.github.mojira.arisa.modules.openHttpGetInputStream import org.slf4j.LoggerFactory import java.io.File @@ -83,7 +84,7 @@ object HelperMessageService { return data.messages[key]?.find { isProjectMatch(project, it.project) } .rightIfNotNull { Error("Failed to find message for key $key under project $project") } .map { localizeValue(it.message, it.localizedMessages, lang) } - .map { resolvePlaceholder(it, filledText) } + .map { resolvePlaceholder(it, filledText?.let(::sanitizeCommentArg)) } .map { resolveVariables(it, project, lang) } } diff --git a/src/main/kotlin/io/github/mojira/arisa/infrastructure/jira/Functions.kt b/src/main/kotlin/io/github/mojira/arisa/infrastructure/jira/Functions.kt index 694386f40..0cb044d63 100644 --- a/src/main/kotlin/io/github/mojira/arisa/infrastructure/jira/Functions.kt +++ b/src/main/kotlin/io/github/mojira/arisa/infrastructure/jira/Functions.kt @@ -408,3 +408,10 @@ fun markAsFixedWithSpecificVersion(context: Lazy, fixVersion fun changeReporter(context: Lazy, reporter: String) { context.value.update.field(Field.REPORTER, reporter) } + +// Allows some basic Jira formatting characters to be used by helper message arguments; +// when used by malicious user they should at most cause text formatting errors +private val sanitizationRegex = Regex("[^a-zA-Z0-9\\-+_#*?.,; ]") +fun sanitizeCommentArg(arg: String): String { + return arg.replace(sanitizationRegex, "?") +} diff --git a/src/main/kotlin/io/github/mojira/arisa/modules/AttachmentModule.kt b/src/main/kotlin/io/github/mojira/arisa/modules/AttachmentModule.kt index 5b9118c48..d49d04cfe 100644 --- a/src/main/kotlin/io/github/mojira/arisa/modules/AttachmentModule.kt +++ b/src/main/kotlin/io/github/mojira/arisa/modules/AttachmentModule.kt @@ -6,6 +6,7 @@ import arrow.syntax.function.partially1 import io.github.mojira.arisa.domain.Attachment import io.github.mojira.arisa.domain.CommentOptions import io.github.mojira.arisa.domain.Issue +import io.github.mojira.arisa.infrastructure.jira.sanitizeCommentArg import java.time.Instant class AttachmentModule( @@ -28,9 +29,11 @@ class AttachmentModule( } } - private fun List.getCommentInfo() = this - .map { "- [~${it.uploader!!.name}]: ${it.name}" } - .joinToString(separator = "\n") + private fun List.getCommentInfo() = this.joinToString(separator = "\n") { + val uploaderName = it.uploader!!.name?.let(::sanitizeCommentArg) + val attachmentName = sanitizeCommentArg(it.name) + "- [~$uploaderName]: $attachmentName" + } private fun endsWithBlacklistedExtensions(extensionBlackList: List, name: String) = extensionBlackList.any { name.endsWith(it) } diff --git a/src/main/kotlin/io/github/mojira/arisa/modules/CommandModule.kt b/src/main/kotlin/io/github/mojira/arisa/modules/CommandModule.kt index da0e22000..d216559fd 100644 --- a/src/main/kotlin/io/github/mojira/arisa/modules/CommandModule.kt +++ b/src/main/kotlin/io/github/mojira/arisa/modules/CommandModule.kt @@ -46,9 +46,7 @@ class CommandModule( } .filter { it.second.isNotEmpty() } .onEach { invocation -> - val commandResults = invocation.second - .map { it.source.line to executeCommand(it) } - .toMap() + val commandResults = invocation.second.associate { it.source.line to executeCommand(it) } editInvocationComment(invocation.first, commandResults) } assertNotEmpty(commands).bind() @@ -119,8 +117,15 @@ class CommandModule( """.trimMargin() } - private fun userIsVolunteer(comment: Comment) = - comment.getAuthorGroups()?.any { it == "helper" || it == "global-moderators" || it == "staff" } ?: false + private fun userIsVolunteer(comment: Comment): Boolean { + // Ignore comments from the bot itself to prevent accidental infinite recursion and command + // injection by malicious user + if (comment.author.name == botUserName) { + return false + } + + return comment.getAuthorGroups()?.any { it == "helper" || it == "global-moderators" || it == "staff" } ?: false + } private fun isStaffRestricted(comment: Comment) = comment.visibilityType == "group" && (comment.visibilityValue == "staff" || comment.visibilityValue == "helper") diff --git a/src/main/kotlin/io/github/mojira/arisa/modules/commands/ListUserActivityCommand.kt b/src/main/kotlin/io/github/mojira/arisa/modules/commands/ListUserActivityCommand.kt index f2211ac14..be36c1f06 100644 --- a/src/main/kotlin/io/github/mojira/arisa/modules/commands/ListUserActivityCommand.kt +++ b/src/main/kotlin/io/github/mojira/arisa/modules/commands/ListUserActivityCommand.kt @@ -2,6 +2,7 @@ package io.github.mojira.arisa.modules.commands import arrow.core.Either import io.github.mojira.arisa.domain.Issue +import io.github.mojira.arisa.infrastructure.jira.sanitizeCommentArg /** * How many tickets should be listed at max. @@ -20,19 +21,22 @@ class ListUserActivityCommand( | OR issueFunction IN fileAttached("by '$escapedUserName'")""" .trimMargin().replace("[\n\r]", "") + val sanitizedUserName = sanitizeCommentArg(userName) + val tickets = when (val either = searchIssues(jql, ACTIVITY_LIST_CAP)) { - is Either.Left -> throw CommandExceptions.CANNOT_QUERY_USER_ACTIVITY.create(userName) + is Either.Left -> throw CommandExceptions.CANNOT_QUERY_USER_ACTIVITY.create(sanitizedUserName) is Either.Right -> either.b } if (tickets.isNotEmpty()) { issue.addRawRestrictedComment( - "User \"$userName\" left comments on the following tickets:\n* ${tickets.joinToString("\n* ")}", + "User \"$sanitizedUserName\" left comments on the following tickets:" + + "\n* ${tickets.joinToString("\n* ")}", "staff" ) } else { issue.addRawRestrictedComment( - """No unrestricted comments from user "$userName" were found.""", + """No unrestricted comments from user "$sanitizedUserName" were found.""", "staff" ) } diff --git a/src/main/kotlin/io/github/mojira/arisa/modules/commands/RemoveContentCommand.kt b/src/main/kotlin/io/github/mojira/arisa/modules/commands/RemoveContentCommand.kt index f4f41fa58..b32ccb9e4 100644 --- a/src/main/kotlin/io/github/mojira/arisa/modules/commands/RemoveContentCommand.kt +++ b/src/main/kotlin/io/github/mojira/arisa/modules/commands/RemoveContentCommand.kt @@ -2,6 +2,7 @@ package io.github.mojira.arisa.modules.commands import arrow.core.Either import io.github.mojira.arisa.infrastructure.escapeIssueFunction +import io.github.mojira.arisa.infrastructure.jira.sanitizeCommentArg import io.github.mojira.arisa.log import net.rcarz.jiraclient.Attachment import net.rcarz.jiraclient.Comment @@ -70,7 +71,7 @@ class RemoveContentCommand( .trimMargin().replace("[\n\r]", "") val ticketIds = when (val either = searchIssues(jql, REMOVABLE_ACTIVITY_CAP)) { - is Either.Left -> throw CommandExceptions.CANNOT_QUERY_USER_ACTIVITY.create(userName) + is Either.Left -> throw CommandExceptions.CANNOT_QUERY_USER_ACTIVITY.create(sanitizeCommentArg(userName)) is Either.Right -> either.b } @@ -79,7 +80,7 @@ class RemoveContentCommand( issue.addRawRestrictedComment( "Removed ${result.removedComments} comments " + - "and ${result.removedAttachments} attachments from user \"$userName\".", + "and ${result.removedAttachments} attachments from user \"${sanitizeCommentArg(userName)}\".", "staff" ) } diff --git a/src/test/kotlin/io/github/mojira/arisa/infrastructure/HelperMessagesTest.kt b/src/test/kotlin/io/github/mojira/arisa/infrastructure/HelperMessagesTest.kt index 5631553c0..cfe0bbde5 100644 --- a/src/test/kotlin/io/github/mojira/arisa/infrastructure/HelperMessagesTest.kt +++ b/src/test/kotlin/io/github/mojira/arisa/infrastructure/HelperMessagesTest.kt @@ -198,6 +198,22 @@ class HelperMessagesTest : StringSpec({ result.b shouldBe "With MC-4" } + "should sanitize filled text" { + val maliciousFilledText = "bad\n\r\n\u0000\"'\u202E" + val result = HelperMessageService.getSingleMessage("MC", "with-placeholder", maliciousFilledText) + + result.shouldBeRight() + result.b shouldBe "With bad???????" + } + + "should allow basic formatting for filled text" { + val filledText = "test *a* and _b_" + val result = HelperMessageService.getSingleMessage("MC", "with-placeholder", filledText) + + result.shouldBeRight() + result.b shouldBe "With test *a* and _b_" + } + "should use the original value when the lang doesn't exist" { val result = HelperMessageService.getSingleMessage("MC", "normal", lang = "cd") diff --git a/src/test/kotlin/io/github/mojira/arisa/modules/AttachmentModuleTest.kt b/src/test/kotlin/io/github/mojira/arisa/modules/AttachmentModuleTest.kt index 0a3930e5f..7e291a9ee 100644 --- a/src/test/kotlin/io/github/mojira/arisa/modules/AttachmentModuleTest.kt +++ b/src/test/kotlin/io/github/mojira/arisa/modules/AttachmentModuleTest.kt @@ -3,11 +3,13 @@ package io.github.mojira.arisa.modules import io.github.mojira.arisa.domain.CommentOptions import io.github.mojira.arisa.utils.mockAttachment import io.github.mojira.arisa.utils.mockIssue +import io.github.mojira.arisa.utils.mockUser import io.kotest.assertions.arrow.either.shouldBeLeft import io.kotest.assertions.arrow.either.shouldBeRight import io.kotest.core.spec.style.StringSpec import io.kotest.matchers.booleans.shouldBeTrue import io.kotest.matchers.shouldBe +import io.kotest.matchers.string.shouldContain import java.time.Instant private val NOW = Instant.now() @@ -57,31 +59,43 @@ class AttachmentModuleTest : StringSpec({ "should comment with attachment details when an attachment is removed" { var removedAttachment = false - var attachmentContent = "" + var comment = "" + var commentRestriction: String? = null val module = AttachmentModule(listOf(".test"), "attach-new-attachment") val attachment = getAttachment( + name = "evil\nAttachment.test", + uploaderName = "evil\nUser", remove = { removedAttachment = true } ) val issue = mockIssue( attachments = listOf(attachment), - addRawRestrictedComment = { it, _ -> attachmentContent = it } + addRawRestrictedComment = { body, restriction -> + comment = body + commentRestriction = restriction + } ) val result = module(issue, NOW) result.shouldBeRight(ModuleResponse) removedAttachment.shouldBeTrue() - attachmentContent.contains(".test").shouldBeTrue() + commentRestriction shouldBe "helper" + // Should contain sanitized user name + comment shouldContain "evil?User" + // Should contain sanitized attachment name + comment shouldContain "evil?Attachment" } }) private fun getAttachment( name: String = "testfile.test", created: Instant = NOW, + uploaderName: String = "someUser", remove: () -> Unit = { } ) = mockAttachment( name = name, created = created, + uploader = mockUser(name = uploaderName), remove = remove, getContent = { ByteArray(0) } ) diff --git a/src/test/kotlin/io/github/mojira/arisa/modules/CommandModuleTest.kt b/src/test/kotlin/io/github/mojira/arisa/modules/CommandModuleTest.kt index d17fb4ed6..4538d2027 100644 --- a/src/test/kotlin/io/github/mojira/arisa/modules/CommandModuleTest.kt +++ b/src/test/kotlin/io/github/mojira/arisa/modules/CommandModuleTest.kt @@ -58,7 +58,7 @@ class CommandModuleTest : StringSpec({ result.shouldBeLeft(OperationNotNeededModuleResponse) } - "should return OperationNotNeededModuleResponse when comment doesnt have correct group" { + "should return OperationNotNeededModuleResponse when comment doesn't have correct group" { val module = CommandModule("ARISA", "userName", ::getDispatcher) val comment = getComment( visibilityType = "notagroup" @@ -72,7 +72,7 @@ class CommandModuleTest : StringSpec({ result.shouldBeLeft(OperationNotNeededModuleResponse) } - "should return OperationNotNeededModuleResponse when comment doesnt have correct value" { + "should return OperationNotNeededModuleResponse when comment doesn't have correct value" { val module = CommandModule("ARISA", "userName", ::getDispatcher) val comment = getComment( visibilityValue = "notagroup" @@ -101,7 +101,7 @@ class CommandModuleTest : StringSpec({ result.shouldBeLeft(OperationNotNeededModuleResponse) } - "should return OperationNotNeededModuleResponse when comment doesnt start with ARISA_" { + "should return OperationNotNeededModuleResponse when comment doesn't start with ARISA_" { val module = CommandModule("ARISA", "userName", ::getDispatcher) val comment = getComment( body = "ARISA" @@ -115,6 +115,23 @@ class CommandModuleTest : StringSpec({ result.shouldBeLeft(OperationNotNeededModuleResponse) } + "should return OperationNotNeededModuleResponse when comment has been written by bot" { + val botName = "botName" + val module = CommandModule("ARISA", botName, ::getDispatcher) + val comment = getComment( + author = mockUser( + name = botName + ) + ) + val issue = mockIssue( + comments = listOf(comment) + ) + + val result = module(issue, RIGHT_NOW) + + result.shouldBeLeft(OperationNotNeededModuleResponse) + } + "should return successfully when comment matches a command and it returns successfully" { var updatedComment = "" val module = CommandModule("ARISA", "userName", ::getDispatcher) diff --git a/src/test/kotlin/io/github/mojira/arisa/modules/commands/ListUserActivityCommandTest.kt b/src/test/kotlin/io/github/mojira/arisa/modules/commands/ListUserActivityCommandTest.kt index c93cd0053..e0cfb5b97 100644 --- a/src/test/kotlin/io/github/mojira/arisa/modules/commands/ListUserActivityCommandTest.kt +++ b/src/test/kotlin/io/github/mojira/arisa/modules/commands/ListUserActivityCommandTest.kt @@ -13,7 +13,7 @@ class ListUserActivityCommandTest : StringSpec({ "should query user activity and post a comment with all tickets" { var calledSearch = false var comment: String? = null - var commentRestrictions: String? = null + var commentRestriction: String? = null val issueList = listOf("MC-1", "MC-12", "MC-1234", "MC-12345") @@ -25,21 +25,53 @@ class ListUserActivityCommandTest : StringSpec({ } val issue = mockIssue( - addRawRestrictedComment = { body, restrictions -> + addRawRestrictedComment = { body, restriction -> comment = body - commentRestrictions = restrictions + commentRestriction = restriction } ) - val result = command(issue, "userName") + val result = command(issue, "user\nName") result shouldBe 1 calledSearch.shouldBeTrue() comment.shouldNotBeNull() - commentRestrictions.shouldBe("staff") + commentRestriction shouldBe "staff" issueList.forEach { - comment.shouldContain(it) + comment shouldContain it } + // Should contain sanitized user name + comment shouldContain "user?Name" + } + + "should post comment when no tickets were found" { + var calledSearch = false + var comment: String? = null + var commentRestriction: String? = null + + val command = ListUserActivityCommand { _, _ -> + Either.fx { + calledSearch = true + emptyList() + } + } + + val issue = mockIssue( + addRawRestrictedComment = { body, restriction -> + comment = body + commentRestriction = restriction + } + ) + + val result = command(issue, "user\nName") + + result shouldBe 1 + calledSearch.shouldBeTrue() + comment.shouldNotBeNull() + commentRestriction shouldBe "staff" + + // Should contain sanitized user name + comment shouldBe "No unrestricted comments from user \"user?Name\" were found." } }) diff --git a/src/test/kotlin/io/github/mojira/arisa/modules/commands/RemoveContentCommandTest.kt b/src/test/kotlin/io/github/mojira/arisa/modules/commands/RemoveContentCommandTest.kt index d2a45185e..14fe310cc 100644 --- a/src/test/kotlin/io/github/mojira/arisa/modules/commands/RemoveContentCommandTest.kt +++ b/src/test/kotlin/io/github/mojira/arisa/modules/commands/RemoveContentCommandTest.kt @@ -8,6 +8,7 @@ import io.kotest.matchers.booleans.shouldBeTrue import io.kotest.matchers.collections.shouldContainExactlyInAnyOrder import io.kotest.matchers.nulls.shouldNotBeNull import io.kotest.matchers.shouldBe +import io.kotest.matchers.string.shouldContain import io.mockk.mockk import net.rcarz.jiraclient.Issue @@ -82,9 +83,11 @@ fun mockRemoveContentCommand( class RemoveContentCommandTest : StringSpec({ "should remove all matching comments" { + val evilUser = "evil\nUser" + var calledSearch = false var comment: String? = null - var commentRestrictions: String? = null + var commentRestriction: String? = null var removedInnocentComments = 0 var removedEvilComments = 0 @@ -94,12 +97,12 @@ class RemoveContentCommandTest : StringSpec({ updateComment = { removedInnocentComments++ } ) val evilComment = MockedComment( - author = "evilUser", + author = evilUser, updateComment = { removedEvilComments++ } ) val restrictedComment = MockedComment( visibilityValue = "staff", - author = "evilUser", + author = evilUser, updateComment = { removedRestrictedComments++ } ) @@ -122,7 +125,7 @@ class RemoveContentCommandTest : StringSpec({ removeAttachment = { removedInnocentAttachments++ } ) val evilAttachment = MockedAttachment( - authorName = "evilUser", + authorName = evilUser, removeAttachment = { removedEvilAttachments++ } ) @@ -150,18 +153,18 @@ class RemoveContentCommandTest : StringSpec({ ) val issue = mockIssue( - addRawRestrictedComment = { body, restrictions -> + addRawRestrictedComment = { body, restriction -> comment = body - commentRestrictions = restrictions + commentRestriction = restriction } ) - val result = command(issue, "evilUser") + val result = command(issue, evilUser) result shouldBe 1 calledSearch.shouldBeTrue() - queriedIssues.shouldContainExactlyInAnyOrder(issues.keys) + queriedIssues shouldContainExactlyInAnyOrder issues.keys removedInnocentComments shouldBe 0 removedEvilComments shouldBe 4 @@ -171,6 +174,8 @@ class RemoveContentCommandTest : StringSpec({ removedEvilAttachments shouldBe 2 comment.shouldNotBeNull() - commentRestrictions.shouldBe("staff") + commentRestriction shouldBe "staff" + // Should contain sanitized user name + comment shouldContain "evil?User" } }) diff --git a/src/test/kotlin/io/github/mojira/arisa/utils/MockDomain.kt b/src/test/kotlin/io/github/mojira/arisa/utils/MockDomain.kt index 09079688b..bc59680aa 100644 --- a/src/test/kotlin/io/github/mojira/arisa/utils/MockDomain.kt +++ b/src/test/kotlin/io/github/mojira/arisa/utils/MockDomain.kt @@ -127,7 +127,7 @@ fun mockIssue( addDupeMessage: (options: CommentOptions) -> Unit = { }, addRestrictedComment: (options: CommentOptions) -> Unit = { }, addNotEnglishComment: (language: String) -> Unit = { }, - addRawRestrictedComment: (body: String, restrictions: String) -> Unit = { _, _ -> }, + addRawRestrictedComment: (body: String, restriction: String) -> Unit = { _, _ -> }, markAsFixedInASpecificVersion: (versionName: String) -> Unit = { }, changeReporter: (reporter: String) -> Unit = { }, addAttachment: (file: File, cleanupCallback: () -> Unit) -> Unit = { _, cleanupCallback -> cleanupCallback() }