@bonnietwin: Would you like to have a look at this one?

I don't use the captive portal but I have set it up in the past on my production IPFire (CU189) to test it out and I uploaded an image for doing that. In the past, around 2022 when I tested it out last, that image was displayed.

Just now I enabled the Captive Portal on my production IPFire blue zone and connected with my laptop to the blue wifi ssid. The connection window was displayed but no image. Also the box for the coupon code entry had no title and the button to press to activate the coupon did not have the word activate in it and in fact there was no text visible anywhere.

So I can confirm that I have reproduced the issue that you have highlighted.

I suspect that in one of the recent core updates some of the WUI layout changes may have caused this to occur.

I will have a look at the code and try and find out what has changed to cause the text and logo to not be displayed and see how to fix it.

I checked the captive portal starting with CU184 and found that the problem of the image not being shown started with CU188.

I also noted that even with CU184 the button to activate the coupon number does not have any text in it, which it should do according to the code. So that stopped working earlier than CU184.

When the image is not shown then the following error message is found in /var/log/httpd/error_log

[cgid:error] [pid 3403:tid 3414] [client 192.168.220.10:49334] malformed header from script 'logo.cgi': Bad header: \xff\xd8\xff\xe0, referer: http://192.168.220.254:1013/

So in CU188 either some existing code was changed to cause this issue to occur or some package was updated and the update causes the problem to occur.

I will now start looking through the CU188 changes and see what I can find.

After a lot of investigation and testing I have found a solution.

It seems that in CU188 something changed that meant that using the simple print statement for providing the content type header to the browser no longer works.

I tested it in some stand alone code and even if using text/txt for the content-type print statement the File::Copy::copy then resulted in an Internal Server Error with the same message as with the image file.

I tested it with text, html, image and application. In all cases the error message about a bad header was provided.

Did some searching and found an alternative way to explicitly print the header info.

With this, in the stand alone code I was able to get an image, html code or text shown in the browser correctly and without any error message.

So I can't figure out and have not been able to find by searching what might have changed to cause the problem but I have found a solution that works and is as described in the perl documentation for providing header info to the browser.

I then used this new method in the logo.cgi code and tested the change in my vm testbed and the image was shown in the captive portal correctly.

I will submit a patch with this change so it can be reviewed by the IPFire dev community.

patch submitted for review

https://lists.ipfire.org/hyperkitty/list/development@lists.ipfire.org/thread/ZZ7EUCQPXXWO2ELG3ZU6N3EGVA2USXX4/

https://patchwork.ipfire.org/project/ipfire/list/?series=4677

I noticed the same problem when playing with the captive portal yesterday for the first time. 
Also found the same 'malformed header' messages in /var/log/httpd/error_log, so I assumed that the content type header was not seen by the webserver. Therefore I simply put a flush after the print statement for the header:

select()->flush();

=> works

I'm not a Perl expert (even not a beginner) and therefore cannot judge whether it is a good or bad solution, but perhaps that's a more suitable solution, than using LibMagic.pm, as discussed in https://lists.ipfire.org/hyperkitty/list/development@lists.ipfire.org/thread/ZZ7EUCQPXXWO2ELG3ZU6N3EGVA2USXX4/

Thanks very much for the feedback. Interesting simple solution. Yes, it looks like something changed in the flushing of the code in Core Update 188, although not sure what package would have caused that.

In terms of its suitability or not, I am also not expert in Perl. @ms would be better placed to answer that.

However I have done some further checking and testing with the code using File-LibMagic.

One thing I found is that in captive.cgi we just check if the image file has a .png, .jpg or .jpeg extension. This is not a secure approach. I was able to create a file with html code inside it, change the extension to .png and successfully upload it to IPFire and with the use of application/octet-stream it would have been sent to the captive users browser.

The code I have worked on with File-LibMagic now checks the actual content type of the file and will only accept png or jpeg type content.

Then in the logo.cgi code I can now extract the content type from the file and only send the header and the file content if it is image/png or image/jpeg.

Currently we have the header content type set to application/octet-stream and this basically means that we are saying this is an unknown binary content and the browser should determine what the content type is and then deal with it appropriately.

That seems very uncontrolled to me. Using the File-LibMagic code to identify the content type from the file itself and then only pass it on to the captive portal users browser if it is a png or jpeg content seems more controlled and more secure to me.

Unless @ms has a different view I plan to continue with the fix with the File-LibMagic code. I have completed a build with this and intend to test it out in my vm testbed system later today/tomorrow to confirm that it works as it should now.

I have just seen from some Mozilla information that with an application/octet-stream content type file browsers are particularly careful when manipulating them to protect users from software vulnerabilities and possible dangerous behaviour.

However I still prefer if the code was explicit in its check, as we define in the captive.cgi WUI page that the image file should be png or jpeg type.

I am happy with only looking at the file extension. It is not the best way to do it, but you are right, there is only a very finite amount of possible image formats that people can use at this time.

I think this problem is not important enough to spend too much development time on it, so the extension check is very good idea that is quick enough to implement.

Did you test what happens if we send nothing?

(In reply to Michael Tremer from comment #9)
> I am happy with only looking at the file extension. It is not the best way
> to do it, but you are right, there is only a very finite amount of possible
> image formats that people can use at this time.
> 
> I think this problem is not important enough to spend too much development
> time on it, so the extension check is very good idea that is quick enough to
> implement.

Already spent the development time on it. I have created the code with File-LibMagic and got it working.

However no problem to leave that and just do the change identified by @Georg

> 
> Did you test what happens if we send nothing?

If you mean if an empty file is uploaded. No I haven't tested that yet.

I will do so.

So I just tried adding the

select()->flush();

after the print statement for the header, in the self contained logo.cgi test code I put together.

My result was that the browser then insisted on asking me to download the logo.cgi file and not showing it on the browser page. If I downloaded it then it was the image file I had provided. It was also now called logo.cgi and not logo.dat.

So something very peculiar occurred for me, which I don't understand at the moment.

If I go back to my

print $q->header(

approach it works fine for me.

I will try it out with the Captive Portal on a vm testbed system and see if it works there for me.

(In reply to Adolf Belka from comment #10)
> (In reply to Michael Tremer from comment #9)
> > I am happy with only looking at the file extension. It is not the best way
> > to do it, but you are right, there is only a very finite amount of possible
> > image formats that people can use at this time.
> > 
> > I think this problem is not important enough to spend too much development
> > time on it, so the extension check is very good idea that is quick enough to
> > implement.
> 
> Already spent the development time on it. I have created the code with
> File-LibMagic and got it working.
> 
> However no problem to leave that and just do the change identified by @Georg

Oh, in that case I lost the plot. This is of course the preferred option.

Update patch set using File-LibMagic has superseded the previous patch.

https://lists.ipfire.org/hyperkitty/list/development@lists.ipfire.org/thread/MY737TMWWUZ7CAUN7OMDBF2P5ICTJWPR/

https://patchwork.ipfire.org/project/ipfire/list/?series=4681

Tested out and confirmed on my vm testbed.

Updated patch set has been merged into next.

https://www.ipfire.org/blog/ipfire-2-29-core-update-192-is-available-for-testing

Have tested out the fix and the image is shown again.

Also trying to enter an html file but with a .png extension gives the error message that the filetype is wrong and shows what the filetype is.

So the fix is working and it will only allow png or jpg filetype files as defined in the WUI page.