Feat: phishing detection #9610

hamza221 · 2024-04-29T12:35:32Z

lib/Service/PhishingDetectionService.php

ChristophWurst · 2024-05-22T15:09:05Z

lib/Service/PhishingDetectionService.php

+	}
+
+	private function isLink(string $text): bool {
+		$pattern = '/^(https?:\/\/|www\.|[a-zA-Z0-9-]+\.[a-zA-Z]{2,})/i';


https://www.php.net/manual/en/function.filter-var.php#refsect1-function.filter-var-examples

+1 for not using a custom Regexp.

But if I understand correctly this method isn't supposed to match only fully qualified URLs, but all text that looks roughly like a URL (as part of a link text).

If I'm correct then FILTER_VALIDATE_URL would be too strict because it doesn't accept e.g. cloud.nextcloud.com/p/something. Additionally it doesn't accept URLs containing non-ASCII characters.

I'd suggest to use a library to find URLs in text. The Horde lib "Text_Filter" contains a class that links all found URLs, maybe some methods of that can be used?

Also maybe the method could be renamed to something more telling (e.g. "textLooksLikeALink()"), and get a comment about its purpose?

https://gist.github.com/gruber/8891611 maybe this can be a more robust solution ?

Text_Filter is based on it

You could use the original source, yes. But a library might bring the benefit that possible changes (there's a list of TLDs in there, which may change) are tended for by multiple people/projects – I'm unsure about the Horde libs in this regard, though.

ChristophWurst · 2024-05-22T15:18:34Z

lib/Service/PhishingDetectionService.php

+use OCA\Mail\AddressList;
+use OCA\Mail\Contracts\ITrustedSenderService;
+
+class PhishingDetectionService {


this new class needs tests

lib/Service/PhishingDetectionService.php

pabzm · 2024-05-23T07:55:38Z

lib/Service/PhishingDetectionService.php

+		}
+		foreach ($zippedArray as $zipped) {
+			if($this->isLink($zipped['linkText'])) {
+				if (str_contains($zipped['linkText'], $zipped['href']) === false) {


I would suggest to use a library to normalize the href-content, because encodings and funny details (e.g. data-URLs) can easily break this comparison even if both parts actually reference the same resource.

Also currently this check would flag a link that e.g. doesn't contain its URL-scheme in its link text, which seems a bit harsh to me? (Example that would be flagged: <a href="https://cloud.nextcloud.com/">cloud.nextcloud.com</a>).

pabzm · 2024-05-23T08:01:43Z

lib/Service/PhishingDetectionService.php

+	}
+
+	private function isLink(string $text): bool {
+		$pattern = '/^(https?:\/\/|www\.|[a-zA-Z0-9-]+\.[a-zA-Z]{2,})/i';


+1 for not using a custom Regexp.

But if I understand correctly this method isn't supposed to match only fully qualified URLs, but all text that looks roughly like a URL (as part of a link text).

If I'm correct then FILTER_VALIDATE_URL would be too strict because it doesn't accept e.g. cloud.nextcloud.com/p/something. Additionally it doesn't accept URLs containing non-ASCII characters.

I'd suggest to use a library to find URLs in text. The Horde lib "Text_Filter" contains a class that links all found URLs, maybe some methods of that can be used?

Also maybe the method could be renamed to something more telling (e.g. "textLooksLikeALink()"), and get a comment about its purpose?

lib/Service/PhishingDetectionService.php

pabzm · 2024-05-23T08:27:45Z

lib/Service/PhishingDetectionService.php

+			];
+		}
+		foreach ($zippedArray as $zipped) {
+			if($this->isLink($zipped['linkText'])) {


This matches only if the link text only contains one URL-like "word". An anchor element with a link text like "login at cloud.nextcloud.com" would not be checked in this code.

kesselb · 2024-05-28T14:05:04Z

Great presentation today 👍

Sorry, I didn't have the opportunity to review the pr.

I'm uncertain about the reply-to warning. The purpose of reply-to is to send the response to a different email. I wonder if we want to use a less scary warning for such a case. For example, our calendar invitations are also using reply-to.
The PhishingDetectionService reminds me a bit about our CheckSetupController in server. I think we could move every check to an own class. Testing is also easier than. Wdyt @ChristophWurst?

Signed-off-by: Hamza Mahjoubi <hamzamahjoubi221@gmail.com>

st3iny

Did not test but left some comments on the code.

lib/IMAP/ImapMessageFetcher.php

lib/PhishingDetectionResult.php

lib/Service/PhishingDetection/ReplyToCheck.php

lib/PhishingDetectionList.php

lib/IMAP/ImapMessageFetcher.php

lib/PhishingDetectionList.php

lib/PhishingDetectionResult.php

lib/Service/PhishingDetection/DateCheck.php

lib/Address.php

lib/Service/PhishingDetection/PhishingDetectionService.php

Signed-off-by: Hamza Mahjoubi <hamzamahjoubi221@gmail.com>

ChristophWurst · 2024-06-04T08:34:45Z

lib/Service/PhishingDetection/PhishingDetectionService.php

+		$this->customEmailCheck = $customEmailCheck;
+		$this->dateCheck = $dateCheck;
+		$this->replyToCheck = $replyToCheck;
+		$this->list = new PhishingDetectionList();


This looks like a temporary variable. Move it into checkHeadersForPhishing or anywhere else it's used.

The current way introduces side effects and if you call checkHeadersForPhishing twice you get outdated results too from the previous invocation.

Signed-off-by: Hamza Mahjoubi <hamzamahjoubi221@gmail.com>

ChristophWurst added enhancement 2. developing labels Apr 30, 2024

hamza221 added 3. to review and removed 2. developing labels May 22, 2024

hamza221 marked this pull request as ready for review May 22, 2024 14:58

hamza221 requested a review from ChristophWurst as a code owner May 22, 2024 14:58

hamza221 requested review from kesselb, pabzm, SebastianKrupinski, ChristophWurst and st3iny and removed request for ChristophWurst May 22, 2024 14:58

ChristophWurst reviewed May 22, 2024

View reviewed changes

lib/Service/PhishingDetectionService.php Outdated Show resolved Hide resolved

ChristophWurst reviewed May 22, 2024

View reviewed changes

pabzm reviewed May 23, 2024

View reviewed changes

hamza221 added 2. developing and removed 3. to review labels May 24, 2024

hamza221 added 11 commits May 29, 2024 18:20

Feat: phishing detection

7e04774

Signed-off-by: Hamza Mahjoubi <hamzamahjoubi221@gmail.com>

fixup! Feat: phishing detection

b6430ab

Signed-off-by: Hamza Mahjoubi <hamzamahjoubi221@gmail.com>

fixup! Feat: phishing detection

48df6ed

Signed-off-by: Hamza Mahjoubi <hamzamahjoubi221@gmail.com>

fixup! Feat: phishing detection

86ff903

Signed-off-by: Hamza Mahjoubi <hamzamahjoubi221@gmail.com>

fixup! Feat: phishing detection

7a296b7

Signed-off-by: Hamza Mahjoubi <hamzamahjoubi221@gmail.com>

fixup! Feat: phishing detection

e5249c0

Signed-off-by: Hamza Mahjoubi <hamzamahjoubi221@gmail.com>

fixup! Feat: phishing detection

a855999

Signed-off-by: Hamza Mahjoubi <hamzamahjoubi221@gmail.com>

fixup! Feat: phishing detection

254467c

Signed-off-by: Hamza Mahjoubi <hamzamahjoubi221@gmail.com>

fixup! Feat: phishing detection

36ab7fd

Signed-off-by: Hamza Mahjoubi <hamzamahjoubi221@gmail.com>

fixup! Feat: phishing detection

b6926aa

Signed-off-by: Hamza Mahjoubi <hamzamahjoubi221@gmail.com>

fixup! Feat: phishing detection

749099d

Signed-off-by: Hamza Mahjoubi <hamzamahjoubi221@gmail.com>

fixup! Feat: phishing detection

88e87cd

Signed-off-by: Hamza Mahjoubi <hamzamahjoubi221@gmail.com>

hamza221 force-pushed the feat/phishing-detection branch from 71db744 to 88e87cd Compare May 29, 2024 16:21

fixup! Feat: phishing detection

b3d696b

Signed-off-by: Hamza Mahjoubi <hamzamahjoubi221@gmail.com>

hamza221 mentioned this pull request May 29, 2024

Phishing detection #9453

Open

15 tasks

st3iny requested changes Jun 3, 2024

View reviewed changes

lib/Service/PhishingDetection/PhishingDetectionService.php Show resolved Hide resolved

lib/Service/PhishingDetection/PhishingDetectionService.php Outdated Show resolved Hide resolved

hamza221 force-pushed the feat/phishing-detection branch from 0c914e1 to b3d696b Compare June 3, 2024 17:23

fixup! Feat: phishing detection

450ceaf

Signed-off-by: Hamza Mahjoubi <hamzamahjoubi221@gmail.com>

hamza221 force-pushed the feat/phishing-detection branch from 815468d to 450ceaf Compare June 3, 2024 17:32

fixup! Feat: phishing detection

60f8348

Signed-off-by: Hamza Mahjoubi <hamzamahjoubi221@gmail.com>

ChristophWurst reviewed Jun 4, 2024

View reviewed changes

fixup! Feat: phishing detection

a134ca1

Signed-off-by: Hamza Mahjoubi <hamzamahjoubi221@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: phishing detection #9610

Feat: phishing detection #9610

hamza221 commented Apr 29, 2024 •

edited by ChristophWurst

ChristophWurst May 22, 2024

pabzm May 23, 2024

hamza221 May 24, 2024

hamza221 May 24, 2024

pabzm May 28, 2024

ChristophWurst May 22, 2024

pabzm May 23, 2024

pabzm May 23, 2024

pabzm May 23, 2024

kesselb commented May 28, 2024 •

edited

st3iny left a comment

ChristophWurst Jun 4, 2024

Feat: phishing detection #9610

Are you sure you want to change the base?

Feat: phishing detection #9610

Conversation

hamza221 commented Apr 29, 2024 • edited by ChristophWurst

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kesselb commented May 28, 2024 • edited

st3iny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamza221 commented Apr 29, 2024 •

edited by ChristophWurst

kesselb commented May 28, 2024 •

edited