I have tested this item ✅ successfully on ef76d64

I am marking this as successful test as it does what it says but its not a very friendly experience as there is no clue what folders are allowed and which are not.

Please consider changing the field to type folderlist

@brianteeman I agree a folder list would be more comfortable. Right now it is the same here as it is in the media manager options, so it's consistent with that. I think if we change it, we should change it also there.

agree - thats why I marked it a successful test and that can be changed in both places in another pr

I have tested this item ✅ successfully on ef76d64

RTC

chmst removed PR-4.0-dev Release Blocker labels 6 minutes ago

This is still a release blocker until tested and merged

@PhilETaylor The issue tracker bot has removed it when setting RTC. I‘ve added it back.

I have tested this item ? unsuccessfully on ef76d64

@PhilETaylor What's not working?

@PhilETaylor If you think your issue #34236 justifies to give this PR here a negative test without any further explanation then I think you are wrong because this PR still fixes the issue as described. If it doesn’t, then please report back your observations.

patience... Im typing as fast as I can!

(sorry, slow to type up the results below, I'm not well, but wanted to mark as unsuccessful test quickly to prevent merge).

Unfortunately this fix is not enough.

1. Side issue of no consequence is here #34236

2. The reason this PR is not good enough is that the validation when using the value from the database and sending the the actual email means that I can still send /etc/passwd file to myself.

This means if a hacker can SQL inject, he can attach any files files to any email.

Using your PR code, I was able to still attach /etc/passwd to my emails.

My exact replication is this.

• My JPATH_BASE is /application
• Set the Attachments folder to be media (which any sane Joomla admin could do)
• Edit the test email, drop down the files and you will see the index.html (from the media folder) - right click and inspect that field, set the value to be relative to your media folder (so in my case ../../etc/passwd) - this replicates a SQL injection (but also shows that you can do this by editing and manipulating the field in Joomla admin, which again you should not be able to do!)
• Go to global config and click test mail - look at the email.

1. Need to validate the file names when saving the Mail Template, the value selected in the dropdown should be a valid file on the hard disk in the attachment folder (in my case media) - not just checking if it is a is_file($path.$file) (not sure if ANY CHECKS at all are actually done on that input when saving?

2. Need to REVALIDATE the filename after pulling it back from the database and when using it in MailTemplate.php just before attaching it to the email.

At the moment you have this code, my notes on this code are comments below. This code is from /libraries/src/Mail/MailTemplate.php

// In my case $path is now JPATH_BASE/media/
		$path = Path::check(JPATH_ROOT . '/' . $config->get('attachment_folder') . '/');

		foreach ((array) json_decode($mail->attachments)  as $attachment)
		{

// need a check here to test if $attachment->file is a file IN THE FOLDER `attachment_folder`. It should be a single 
// filename with no path at all. `filename.extension` only, and should exist in the array of 
//    `scandir(JPATH_ROOT . '/' . $config->get('attachment_folder'))` - rather than using `is_file` to check if the file exists. 

// At the moment, here $path . $attachment->file = JPATH_BASE/media/../../etc/passwd
 // So this is TRUE - thats bad! 
			if (is_file($path . $attachment->file))
			{
// and then the file is attached, even though its not in the attachment_folder. $attachment->file = JPATH_BASE/media/../../etc/passwd
				$this->mailer->addAttachment($path . $attachment->file, $attachment->name ?? $attachment->file);
			}
		}
``

Also, if I dont provide an attachments_folder in options - the root JPATH_BASE folder is used... so... I ... can email my configuration.php :) :)

@PhilETaylor As as I see you describe a new exploit now. The one you‘ve described in issue #34185 is solved here.

If you specify an empty value you should not see any fields for attaching files in the Mail template form. Please test this and not just make assumptions.

If you specify an empty value you should not see any fields for attaching files in the Mail template form. Please test this and not just make assumptions.

Is my screenshot not enough proof for you that I tested it??

Would a video prove it to you?

If you specify an empty value you should not see any fields for attaching files in the Mail template form. Please test this and not just make assumptions.

Hmm, no, that might be wrong. But I‘m too tired now.

Back to pending.

@PhilETaylor As as I see you describe a new exploit now. The one you‘ve described in issue #34185 is solved here.

No - the exact same bug - just expounding on it.

The only thing that is fixed, is now you cannot see the files from the /etc/ folder in a select box. You can still manually type a value in and use it, but you cannot see the file names.

None of this is preventing me from entering a file as the attachment for a specific email that is in a forbidden location

As I repeatedly have pointed out the validation of data is missing in many places and quite often (as seen here) relies 100% on someone not manipulaating the form before submitting it. Instead of validating the actual data ssubmitted it assumes that if you submitted it then its ok.

Please consider changing the field to type folderlist

@brianteeman I've just checked that and it seems a folderlist is just a list of folders in a parent folder without the possibility to go deeper. Maybe you meant a media field, where we can navigate through a folder structure? If so, then it might be that this is limited to a base folder defined in media manager options. But with this I'm not sure.

inspect the form and change the value of one of the possible files to a file and path that you should not have access to.
Save and then check the stored value for the attachment in the database

inspect the form and change the value of one of the possible files to a file and path that you should not have access to.
Save and then check the stored value for the attachment in the database

@brianteeman I guess that doesn't refer to my comment before yours where I asked about the folderlist field and media field.

folderlist field and media field.

Are the submitted values of those fields actually validated on save? Else if they can be manipulated in the dom we are back to square one

folderlist field and media field.

Are the submitted values of those fields actually validated on save? Else if they can be manipulated in the dom we are back to square one

@PhilETaylor I think with the media field I was wrong because it's used to select files and not folders, and the folderlist is not appropriate for the reason I stated above, so we forget about these now.

I will see if I can add validation for the other issues here but it's late now and I'm tired and gotta work tomorrow so it may take time. If someone is faster: Go for it.

There is no rush. I'm in bed too and have a flight plan filed for 10am so need to sleep. Thanks for working on this, I'm hoping to have the weekend off this week haha

folderlist field and media field.

Are the submitted values of those fields actually validated on save? Else if they can be manipulated in the dom we are back to square one

Not that I could see

I just see folderlist can be recursive, so it would really be more comfortable here where we want to be limited to the Joomla root or below. Of course it still would need to do some validation in the model before saving and more. Will work on that tomorrow or later.

May I make a suggestion.
The ability to send an attachment is a new feature. Instead of taking up a lot of time in the last few days before RC trying to secure it just pull it out of 4.0.
Then it can be worked on at leisure for 4.1
No one will miss a feature that they never had and that has questionable benefits. Have you ever wanted to add an attachment to one of Joomla's automated emails? I haven't.

I think I can fix it, but if I don‘t get it fixed until weekend we can wipe it out.

@richard67 I made a PR to your branch richard67#17 for some more validations. Could you please review?

@joomdonation Thanks for the PR, but I was already working on a slightly different way to solve it. It does the same but with a bit different code.

Please let us know when it is ready for retesting

Please let us know when it is ready for retesting

Will do so.

Following has been done with the last changes:

• Input filtering has been added to the attachments folder field of the component's configuration form so that invalid characters or spaces only result in the form being saved with empty value for that field, like it is with other input filters, and this will then result in email attachments are disabled.
• The attachments folder check on loading the mail template form has been improved so that an empty or otherwise bad value is correctly detected and the fields for attachments are not added to the form. This also ensures that it's not possible to browse the Joomla root folder for adding attachments.
• The attachments folder check on sending email has been improved so that it's not possible to attach files from the Joomla root folder.
• For each attachment, the complete path is checked so that it's not possible anymore to send email with not existing attachments.

Following remains to be done:

• I think about adding a custom exception so we see a useful exception message when sending email is aborted due to invalid folder or file for attachments (which at this point could only come from a bad parameter in database from past or a hack).
• Testing instructions need to be adapted to changes.

I have to test it on Windows, too, not only on Linux. Stay tuned.

Ok, I'm ready with the folder and the file name on server side.

But as @brianteeman correctly observed, you can enter in the mail template form for an attachment anything you want in the text field labelled with "File Name". And this field is then used as the file name for the attachment in the email, so when you try to save that file after having received the email, you get proposed the weird and possibly (depending on the OS where the email client is used) invalid name.

So it needs some validation and maybe also input filtering for that field, too.

... And that field should not allow to change the extension. Not sure if I can fix that here or if it is better to make a separate PR for that.

@richard67 that wasn't quite what I meant but its a valid concern

I have checked what I can with this and it appears to be ok now but I am in no way an expert at this type of thing.

(still not convinced on the value of this feature)

@brianteeman Currently you can with my PR now safely chose a file for attachment and then enter a completely weird file name with which it will be attached in the email. That means we can attach a file "virus.exe" from the server to the email with a name "naked-girl.jpg" and when the received wants to check the attachment it is "naked-girl.jpg" (or on Windows when hiding extensions of known file types just "naked-girl") ?

@brianteeman P.S. What I describe in my previous comment is a very valuable feature for certain people ? I just don't think they should use Joomla for their business.

in the context of the mail templates that we have I just dont see the use case

I think I have to revert the input filter because it disallows folder names with non ASCII characters, e.g. Greece or Russian letters.

@Quy I've refactored the code and followed your suggestion to move the loop inside the if even if I'm not a fan of too deeply nested if's, but it's a matter of taste.

@joomdonation You should be happy too because I've decided to use trim as you had suggested after I had to revert the input filtering.

Will update testing instructions a bit later and report back when it's ready for testing.

@brianteeman @joomdonation @PhilETaylor @Quy (in alphabetical order): Ready for testing. Please test. Many test steps in the instructions, but less time consuming than it looks like.

Is it the intended behaviour that if you select a file from the list called fred.jpg and enter a filename of mary.png the attachment is named as mary.png.jpg ?

As stated before the user experience of setting the folder in the options is very frustrating as you dont know why the folder input was rejected

otherwise I have checked what I can with this and it appears to be ok now but I am in no way an expert at this type of thing.

Is it the intended behaviour that if you select a file from the list called fred.jpg and enter a filename of mary.png the attachment is named as mary.png.jpg ?

@brianteeman Yes. As described in the description, this PR makes sure that you can't send the attachment with a different file name extension than the real file has (if it has one).

As stated before the user experience of setting the folder in the options is very frustrating as you dont know why the folder input was rejected

I fully agree, but I can't fix this with this PR here.

@brianteeman So was your test a successful test or not? Am asking because I see no result marked in the tracker.

It is neither. I didn't find anything wrong but I don't feel qualified enough to say this is secure

/me is typing...

Another option than fixing this attachment folder option for mail templates would be to make it use the media field to select attachments and remove that attachments folder option from the mail component's options. But that would be a bigger change and need more time, so I just wanted to quickly fix the security hole.

Sorry for my absence/delay. Some thoughts on the current state after this PR would be merged...

Sending email will be rejected with an exception if the "Attachments Folder" is not valid or equal to the Joomla root or if the full path to the attachment file is not valid or the file doesn't exist.

Finally the path is checked when attaching files to mails on sending. If there for some reason the path is not valid, an exception is raised, so sending the mail is aborted.

1. This exception is unhandled and comes from Path::check - That might be ok... , but in Global Configuration it displays as a HTTP Error and not an exception message when sending the test mail.

It would be interesting to see other places in Joomla handling the failure to send a Mail Template because of this exception being thrown and not handled... Its not like Joomla to throw an unhandled exception.

2. If your provided custom file name ends in a period, then you get customfilename..jpg as an extension.

3. Out of scope of this PR: validate="filePath" doesnt validate correctly, but for example using ftp://example.com is a valid attachment_folder at the moment - added more examples to #34236

4. I can still right click the dropdown list of files, and change the value of the dropdown, and click save, and its inserted into the db, despite my input being outside of the Joomla site. E.g attachments_folder = images, and I edit the files list and change joomla_black.png to ../../joomla_black.png and can save the Mail Template with no errors. This should not be allowed and still needs fixing. Forked to #34262

5. After doing 4 above, the image is obviously bad as ../../joomla_black.png would take you out of the joomla file system, this causes Path::check to throw an exception Joomla\CMS\Filesystem\Path::check() - Use of relative paths not permitted, which is not caught and handled (see 1), leading to a PHP fatal error (or on my set up, when sending the test email from Global Config, I get a An error has occurred while fetching the JSON data: HTTP 502 status code. Bad Gateway" error message shown on the page) #34263

6. If, after resolving the folder, and resolving the filename, and putting them together and not finding that file on the hard disk - no feedback is provided to say the attachments were not valid, and not attached, and the email is sent without attachments, silently. Im not sure this is what "A Joomla Admin" would expect. If they have a mail that is configured to send an attachment, and it doesnt (because Admin B deleted the file!) then the mail sending should fail right? Sending the mail with no attachment, when one is confiugred, just seems wrong. Happy to be overruled on this.

7. Are you sure the "media field to select attachments." would solve the issue? I doubt that also validates the input on saving...

re 4
I had the same as you in that I could put the invalid data into the database BUT when the email was sent it was ignored and I didnt get an error message

Summary: This PR fixes the security issue.

However further refinement is needed generally, and Joomla should be doing these kinds of code reviews before features are merged to the core, and not 4 weeks after a RC is due. The JSST should be taking an active part in the development of the product and doing proactive code reviews (as it is in their role to do so) and not ignoring Joomla 4.

I have tested this item ✅ successfully on 818407d

Great work. Sometimes when given a pig, bacon sandwiches is all you can smell.

@wilsonge I'd like to ask you to check this PR and decide what to do with it, merge and further improve, or better change this email attachment feature of the mail component to not use an own attachment folder parameter and own file name field but the media field, or even remove this attachment feature.

Personally if we had enough time I'd tend to the latter, i.e. change or remove the feature,

@brianteeman @PhilETaylor Just to be sure you get me right: I agree with all you find which is not good with this function, I just don't have the time and maybe also not the knowledge to fix them all. And yes, we should have reviewed and fixed it long time ago.

This could be merged as it is. That would fix the release blocking security issue.

I could not do a full code review right now. But I hope @wilsonge will do a code review before merging. From my quick look, I have few small complains:

• Why do you use setAttachmentName as method name? Maybe getAttachmentName would be better. set indicates that we set something, but the actual logic here is that we calculate the attachment name base on the real attachment file and the alternative name
• Why do you use $attachment->name ?? $attachment->file in the setAttachmentName call? I think we just need to pass $attachment->name only and in case it empty, just return '' and the the mailer would use the name from $attachment->file. That will save us from some extra unnecessary code in case $attachment->name empty (which I expect in most case)
• We have a library method File::getExt to get file extension. I'm unsure which one is better (compare with pathinfo) but normally, I would use the build in library method if available.

Overall, this PR is good and solve the security issue. I'm unsure if my mentioned issues above are right, but @wilsonge should review and make decision.

Why do you use setAttachmentName

Quick easy change @richard67

Why do you use $attachment->name ?? $attachment->file

These are being passed to setAttachmentName you mentioned above, and not as you think to the addAttachment method call. I think you missed that /me must read before commenting...

@brianteeman @joomdonation @PhilETaylor @Quy (in alphabetical order): Ready for testing again.

I've implemented @joomdonation 's suggestion to rename setAttachmentName to getAttachmentName and have simplified the check for the attachment name (field "Filename") and the file name extension handling. Tesing instructions have been adapted and simplified a bit.

Meanwhile I am working on another PR to implement validation for file and folder paths including the check if the file or folder exists so it would not be possible anymore to enter a not existing folder name for attachments.

Meanwhile I am working on another PR to implement validation for file and folder paths including the check if the file or folder exists so it would not be possible anymore to enter a not existing folder name for attachments

And what if the file or folder is removed later?

And what if the file or folder is removed later?

@brianteeman That's a different thing which requires validation when the folder is used, what this PR here does for the mail attachments. But my new PR will solve @PhilETaylor 's issue that you can enter not existing folders and save the form and get the message that the form is saved and all looks fine.

I have tested this item ✅ successfully on cd0c6d8

The more I look at this the more I ask myself what is the point of the filename field. What purpose does it serve? What is the usecase that it satisfies

@brianteeman You can send an attachment with language-specific file name.

ok - an edge case that I didnt consider but a valid one - thanks

@PhilETaylor Could you please test this PR?

Ive just landed a short while ago and still at the airport. Once Im home and showered its in my list.

@PhilETaylor Could you test this PR again? Thanks in advance.

I have tested this item ✅ successfully on cd0c6d8

This PR is great enough to be merged, a lot of hard work by Richard and the initial security issue is well catered for and fixed now.. Just some notes:

1. #34263 still occurs when trying to send test email from admin when db contains manipulated ../../etc/passwd file name - however, on frontend when trying to attach ../../etc/passwd to an email (tested with password reset) you correctly get "Joomla\CMS\Filesystem\Path::check() - Use of relative paths not permitted" (See #34130) and "Failed sending email."

2. There is a logic issue where, if you set a correct folder, and set a correct attachment file to a mail, and then go an rename the folder on the hard disk - so it no longer exists as configured as the attachment folder, then the Mail Templates when edited no longer show the configured files as attachments (which are still stored in the db). Thats not the fault of this PR

3. Further to 2 above, If there is a set a correct folder, and set a correct attachment file to a mail, and then the folder is inaccessable for some reason - the email is still sent, just with no attachment, and does not throw an exception that it could not find the file to attach. The "Summary of Changes" state:

Sending email will be rejected with an exception [...] if the full path to the attachment file is not valid or the file doesn't exist.

That doesnt happen. (This was point 6 in the comment #34233 (comment))

4. If my filename is "FileWithNoExtensionIsValidTest" and my name is "mine.txt" the file is attached with name "mine". That was not expected, as the original filename had no extension. But Im ok with it being this way.

   ---

   _{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34233.}

@PhilETaylor I've applied your suggestions. Could you and @joomdonation briefly test again? Thanks in advance.

I have tested this item ✅ successfully on e4b46ea

Im still happy after changes made in e4b46ea

@PhilETaylor For point 6 of your comment about check if the folder exists at the end see my new PR #34285 . For the issue #34262 with filelist field, and that applies also to folderlist, too, see my new PR #34284 .

I only mentioned point 6 because it was specifically called out as a change in this PR and therefore expected that this PR would fix it. I was aware that you had other PRs open - thanks for those. Some hard work here while Ive been out having fun...

I'll take these other PR's for a spin shortly. Not sure when @wilsonge is tagging the RC, but this PR absolutely needs to be in the RC, the others not so much so.

RTC

Previous test result are still valid because the changes after that were just changes in code comments, type hint for the new method and code style.

The more I look at this the more I ask myself what is the point of the filename field. What purpose does it serve? What is the usecase that it satisfies

The additional oversight is that placeholders cannot be used in the name field, like they can in the mail body/subject.

Thinking how this feature might be used, the 3pd might want to add dynamic detail to the file name. E.g Joe Blogs buys an ebook, and so the filename becomes Order123_Bloggs_book.pdf dynamically using the placeholders.

Forked to #34287

Thanks!

Thanks all!