Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Liquidsoap specific metadata tags could be set by file #3813

Closed
vitoyucepi opened this issue Mar 23, 2024 · 8 comments · Fixed by #3820
Closed

Liquidsoap specific metadata tags could be set by file #3813

vitoyucepi opened this issue Mar 23, 2024 · 8 comments · Fixed by #3820

Comments

@vitoyucepi
Copy link
Collaborator

Describe the bug
If the file has metadata tags with the same names as the internal tags of liqduidsoap, the internal tags will be suppressed.

To Reproduce

  1. ffmpeg \
      -f lavfi -i "sine=frequency=100:duration=30" \
      -c:a libmp3lame -b:a 128k \
      -metadata title=music_1 \
      -metadata on_air="2024/01/01 00:00:00" \
      -metadata temporary="true" \
      -metadata rid=0 \
      -metadata on_air_timestamp=0.0 \
      -metadata filename="123" \
      -metadata initial_uri="456" \
      -metadata kind="{v" \
      -y 1.mp3
  2. a = single("1.mp3")
    a.on_metadata(print)
    output.dummy(a)

Expected behavior

  1. Metadata should be split into two parts.
    • Tags from the file as [string*string].
    • Internal tags as a record.
  2. Liquidsoap's internal tags should always overwrite tags from the file.

Version details

  • OS: Debian in docker
  • Version: 2.2.4-1

Install method
savonet/liquidsoap:v2.2.4

Common issues
#3782 (reply in thread)

@toots
Copy link
Member

toots commented Mar 24, 2024

That's a good point thanks for reporting. I think we should prefix/namespace our internal tags so that they are clearly identifiable.

@vitoyucepi
Copy link
Collaborator Author

Hi @toots,
Could you make this issue confidential?

@toots
Copy link
Member

toots commented Mar 24, 2024

I don't think that GitHub supports that. I'll setup a private repository when I get back home and will add you to it.

@toots
Copy link
Member

toots commented Mar 25, 2024

The fix is on its way. I could consider taking this offline but I'm not sure how sensitive the issue really is. If an attacker has control over the files sent to the playout system, there's a lot more damage they could do before getting into override the root metadata.

Do you have a specific scenario in mind?

@vitoyucepi
Copy link
Collaborator Author

I think filename or initial_uri could point to a completely different file or contain a shell script that could be executed in process.run if process.quote is not used.

@toots
Copy link
Member

toots commented Mar 25, 2024

Yes absolutely and that is why I have prioritized the fix. However, any one sending files to the system should be considered either a trusted user or else the system should be protected by security measures such as running the script inside a container. We also provide support for sandboxing external processed in such cases: https://www.liquidsoap.info/doc-2.2.4/settings.html#sandboxing-for-external-processes.

@Moonbase59
Copy link

Moonbase59 commented Mar 25, 2024

Don’t forget that we want to tag our files to have the liq_… tags (like cue points, fading, etc.) set by pre-processing, probably even mass-tagging.

So lengthy calculations like autocue2: or autocue: can be avoided by just reading the tags—much faster.

I know many who prefer keeping "data together where it belongs", i.e. cue points belong into the audio file.

Talking about possibly security-relevant tags I agree: These should be kept out, if possible.

@vitoyucepi
Copy link
Collaborator Author

Hi @toots,
Have you considered the following type for metadata?

[ string * string ].{
  initial_uri: string,
  filename: string,
  temporary: bool,
  rid: int,
  on_air: string,
  on_air_timestamp: int,
  kind: string,
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants