mattermost / mattermost-mobile

Next generation iOS and Android apps for Mattermost in React Native
https://about.mattermost.com/
Apache License 2.0
2.25k stars 1.36k forks source link

unsanitized use of user defined input as regex #4388

Closed just-ole closed 1 year ago

just-ole commented 4 years ago

https://github.com/mattermost/mattermost-mobile/blob/master/android/app/src/main/java/com/mattermost/rnbeta/CustomPushNotification.java

    private String removeSenderNameFromMessage(String message, String senderName) {
        return message.replaceFirst(senderName, "").replaceFirst(": ", "").trim();
    }

the user controlled string senderName is passed to String.replaceFirst which expects the first argument to be a regex without any sanitizing which can crash the app by using THE User ;-) as name

but thats all I have time to investigate atm

current version in the play store is affected according to a colleague and according to him the non beta version was crashing before as well

here is the exception he copied from a screenshot and pasted into our channel the only thin I changed was replacing the actual username with the word username but the real username matches [A-Z][a-z]+

java.util.regex.PatternSyntaxException: Incorrectly nested parentheses in regexp pattern near index 15 DER Username ;-)

at java.util.regex.Pattern.compilelmpl(Native Method) at java.util.regex.Pattern.compile(Pattern.java:

1340) at java.util.regex.Pattern.<init>(Pattern.java:1324) at java.util.regex.Pattern.compile(Pattern.java:

946) at java.lang.String.replaceFirst(String.java:2171) at

com.mattermost.rnbeta.CustomPushNotification.r emoveSenderNameFromMessage(CustomPushNot ification.java:528) at

com.mattermost.rnbeta.Custom Push Notification.and Messaging Style Messages(Custom PushNotificat ion.java:374)

at com.mattermost.rnbeta.CustomPushNotification.g etMessagingStyle(CustomPushNotification.java:

309) at com.mattermost.rnbeta.Custom Push Notification.s et Notification MessagingStyle(CustomPushNotifica tion.java:289)

at com.mattermost.rnbeta.Custom PushNotification.g etNotificationBuilder(CustomPushNotification.java:

194) at

com.wix.reactnativenotifications.core.notification.P ushNotification.buildNotification(PushNotification.j ava:181)

at com.wix.reactnativenotifications.core.notification.P ushNotification.postNotification(PushNotification.j ava:128) at

com.mattermost.rnbeta.CustomPushNotification.o nReceived(CustomPushNotification.java:166)

at com.wix.reactnativenotifications.fcm.Fcminstancel dListenerService.on MessageReceived(Fcmlnstanc eldListenerService.java:27)

at com.google.firebase.messaging.FirebaseMessagingService.zzd(Unknown Source:60) at com.google.firebase.iid.zzc.run(Unknown Source:2) at java.util.concurrent.ThreadPoolExecutor.runWorker ThreadPoolExecutor.java:1162) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:636) n(ThreadPoolExecutor.java:636) at com.google.android.gms.common.util.concurrent.z za.run(Unknown Source:7) at java.lang.Thread.run(Thread.java:764)
amyblais commented 4 years ago

@just-ole What device and version are you on?

just-ole commented 4 years ago

got introduced in https://github.com/mattermost/mattermost-mobile/commit/1777a4f7504271fccf123e50589a5edc6ffb8ea3 and is still in master so all versions after that and since I already located the bug in the code I didn't bother to find out which version was the latest in the google play store at that point of opening that issue especially since it should be easier to for you to figure that out yourself than it is for me

enahum commented 4 years ago

@just-ole would you be willing to submit a PR?

enahum commented 4 years ago

Actually we should remove that entire code block, is been there for 18 months or so and now the sender name is set in the notification bundle and no longer in the message.

just-ole commented 4 years ago

haven't wrote a single line of java in over 5 years, so I won't touch anything

larkox commented 1 year ago

Closing since this should be already solved.

just-ole commented 1 year ago

yeah looks like #4707 fixed it