WIP: Add duktape as javascript engine. - [closed] #244

Closed vmihalko closed 11 months ago

In GitLab by @yetist on Jul 24, 2019, 17:53

Merges dev-duktape -> master

I don't know if there is a chance to use duktape as the js backend for polkit.

I saw this email on the mailing list.

But some things have changed now:

Duktape can be found in popular distributions.
- Fedora: 2.2.0
- Archlinux: 2.3.0
- Debian / Ubuntu: 2.3.0

https://pkgs.org/download/duktape

This PR provides a new option --with-duktape to enable build with duktape. If you don't add this option, it will still compile with mozjs as before.
Now it uses the system's duktape dynamic library.
This PR migrates the code to duktape-2.0.0+, and when duktape is updated, it will automatically get a new implementation.

In GitLab by @zx2c4 on Dec 2, 2020, 13:42

@jrybar What's the story looking like to get this merged? Is it likely to make the next polkit release?

In GitLab by @jrybar on Dec 2, 2020, 15:21

See https://gitlab.freedesktop.org/polkit/polkit/-/merge_requests/35#note_705827

In GitLab by @jrybar on Dec 2, 2020, 15:26

One way is to keep both options if the code is cleaned. Packagers can choose during configuration time. However that would mean that mozjs polkitbackend would need to be updated and tweaked every once in a while when new SpiderMonkey comes out and changes something, which is inconvenient.
This brings up a good discussion.

In GitLab by @cihlarma on Dec 2, 2020, 16:04

new SpiderMonkey comes out and changes something

Do they seriously do that...? :face_palm: More emphasis on backwards compatibility is the one and only thing I wish the FOSS projects would learn from Microsoft; I'm not advocating for hacks or keeping legacy code just so that some poorly-written commercial software from 25 years ago still functions but thing "throw it away and re-write it every 3 months" mentality gets ridiculous sometimes.

Is Duktape at least more stable or less dynamic than SpiderMonkey?

In GitLab by @tmccombs on Dec 3, 2020, 09:03

Spidermonkey is the Javascript implementation for Firefox. As I understand it, a backwards compatible API is not a design goal, because the API is primarily intended to be used by the version of firefox it is distributed with, and not really designed to be embedded in other projects (although it can be done).

In GitLab by @yetist on Dec 6, 2020, 07:13

marked this merge request as draft

In GitLab by @yetist on Dec 6, 2020, 07:24

I think it has not been completed yet and it will take time to process the review comments.

In GitLab by @swills on Jan 4, 2021, 22:39

I wouldn't stress over that, plenty of distros are still using old versions, some are using versions where JS is optional I believe:

https://repology.org/project/polkit/badges

In GitLab by @StefanBruens on Jan 5, 2021, 24:26

Actually the only distribution which still uses 0.105 is Debian and derivatives, everyone else has moved on to 0.115 ... 0.118

In GitLab by @tmccombs on Jan 11, 2021, 06:08

For what it's worth, I think the reason debian hasn't upgraded past 0.105, is because they don't want a dependency on mozjs.

In GitLab by @swills on Jan 11, 2021, 16:58

I suspect you're right, and in hindsight, I wish we'd done the same in FreeBSD, personally.

In GitLab by @ferion11 on Jan 12, 2021, 11:16

By the way, about the runaway_script_killer, I don't know if this helps, but I don't think DUK_HTHREAD_INTCTR_DEFAULT is a good idea, because of: https://github.com/svaarala/duktape/issues/1987

so I was thinking of an outside approach using pthreads or Apache Portable Runtime, like: https://stackoverflow.com/questions/7738546/how-to-set-a-timeout-for-a-function-in-c

Perhaps as an option in build time, instead of mandatory. I like more the idea of writing rules that don't have this issue than to use extra code to check if I haven't done the job right. Don't get me wrong, it has its place, and I can use it while making new rules, but not running all the time, I guess.

What do you think?

In GitLab by @swills on Jan 12, 2021, 15:19

Perhaps something as simple as a fork/exec and then using setitimer() to timeout the parse and kill the child process would be sufficient (if it hasn't completed in a reasonable time), simpler and should be reasonably portable.

In GitLab by @genexerc on Feb 6, 2021, 11:26

duktape has internal ability to timeout script execution by using DUK_USE_EXEC_TIMEOUT_CHECK macro but this is must be done during configure time because duktape designed to be kind of embeddable thing. That way duktape (configured?)source must be distributed with polkit because ductape shared library provided by distros does not implement timeout check.

current implementation of runaway_killer throws an exception to js code(sic!) when time out occurs. That may be seen in https://gitlab.freedesktop.org/polkit/polkit/-/blob/master/test/data/etc/polkit-1/rules.d/10-testing.rules#L182

do we need this kind of functionality or just aborting script thread with return error code will be enough?

In GitLab by @ferion11 on Feb 6, 2021, 20:39

As I said before, I don't like using the duktape timeout (there's only an interrupt check every 262144 opcodes, hard-coded on src-input/duk_hthread.h with DUK_HTHREAD_INTCTR_DEFAULT). And in this case, a simpler (and optional) solution would be more convenient.

In GitLab by @ferion11 on Feb 6, 2021, 20:54

We just need to pass the test using timeout. I don't think we should be restricted to a single type of solution. So you can abort the script thread and throw one exception (if you need it).

In GitLab by @genexerc on Feb 8, 2021, 24:45

Actually, what am I trying to say is that that simple case will not work as expected.

I tried to issue make check test going in infinite loop in polkit_backend_js_authority_check_authorization_sync at call to duk_pcall_prop()

I wrapped call as described here and it indeed timed out after 15 seconds but test still fails because it checks for exception to be returned to js code.

// ---------------------------------------------------------------------
// runaway scripts

polkit.addRule(function(action, subject) {
    if (action.id == "net.company.run_away_script") {
        try {
            // The following code will never terminate so the runaway
            // script killer will step in after 15 seconds and throw
            // an exception...
            while (true)
                ;
        } catch (error) {
            if (error == "Terminating runaway script")
                return polkit.Result.YES;
            return polkit.Result.NO;
        }
    }
});

So @jrybar will not accept PR with that implementation.

In GitLab by @genexerc on Feb 8, 2021, 02:01

As about DUK_HTHREAD_INTCTR_DEFAULT its no problem to change interrupt change period if you bundle duktape source with polkit. You need it in any case because DUK_HTHREAD_INTCTR_DEFAULT will not work with shared library bundled with distros because this is config option that must be enabled during duktape compilation.

If we try to use DUK_USE_EXEC_TIMEOUT_CHECK it is possible to implement return-to-js approach but it internally use DUK_HTHREAD_INTCTR_DEFAULT and have much more problems than long period of checks. For example it will not work at all if infinite loop occurs in js wrapped C function.

In GitLab by @ferion11 on Feb 8, 2021, 06:37

I don't think that both are simple solutions. The simple solution I proposed involves first passing the watchdog timeout from the javascript engine to the polkit, after all it's the polkit that should choose whether or not to interrupt the javascript engine for any external reason, be it timeout or not (and of course, my belief that this would be a better software design).

In any case, since 2015 there has been an attempt to create a better mechanism within the duktape (Improve executor timeout mechanism), but so far no success that i'm aware of.

And that is why, I would like to discuss a simpler way to deal with this issue within the polkit.

In GitLab by @swills on May 14, 2021, 21:10

I wrapped call as described here and it indeed timed out after 15 seconds but test still fails because it checks for exception to be returned to js code.

This seems like a good approach, perhaps the test just needs updating?

In GitLab by @genexerc on May 16, 2021, 04:45

This will break some logic in systems which already based on error handling in js(if any). But there is no way to check, because maintainer insists on thorought compatibility between systems.

I think that you need to make a decision first -- what way it will evolve futher. spidermonkey is already replaced by v8 in modern systems(if you want to continue with js). duktape is a dead-end too, because if you want to change config system, you need to add new and then delete old, but not branch and support each.

Personally, I use gentoo and this patch fits perfectly into local overlay as a temporary(lifetime probably) solution.

In GitLab by @tmccombs on May 16, 2021, 09:23

spidermonkey is already replaced by v8 in modern systems

I'm not sure what you mean by that. spidermonkey is very much still in use in Firefox, and looking at the reverse dependencies for the js78 package in Archlinux, looks like it is also used for JS bindings in Gnome and Cinnamon. I also don't know what you mean with your comment that duktape is a dead-end.

In GitLab by @corvus on May 16, 2021, 09:30

And I hope you're not suggesting using v8 for polkit.

In GitLab by @swills on May 16, 2021, 19:58

Maybe JavaScript isn't the best answer for configuration syntax either. Personally, I'd like to see UCL used.

In GitLab by @genexerc on May 16, 2021, 22:07

@tmccombs i mean that scripting possibilities in polkit config is overkill no matter what js engine you are going to use. Its common way to support your arguments like "what the heck! we are already using this js engine in other parts of the system, so it does not bring new deps", but some systems give user more possibilities. And I can say for sure that spidermonkey used only by polkit in my configuration. And thats give you some hints that polkit evolution went wrong somewhere.

In GitLab by @tmccombs on May 17, 2021, 07:18

The policy system before polkit switched to using JS definitely wasn't flexible enough. I'd be fine with switching to something other than javascript, but it would need to be flexible enough that existing policies, even complicated ones, could be reasonably migrated to the new syntax.

In GitLab by @genexerc on May 17, 2021, 19:05

Probably anything turing-complete will fit. js was made for web and many of its functionality looks like overkill here.

If properly asked, users may suggest interesting alternatives. But you need to provide an example of complicated policy to combat with.

In GitLab by @Aduskett on May 17, 2021, 19:22

"spidermonkey is already replaced by v8 in modern systems"

This statement ignoress every single embedded Linux device. The only program that uses spidermonkey on my builds is polkit, which is a 20MB .so that takes almost a gig of space to compile.

If you check out: buildroot

You can see that there are 11 patches just to build spidermonkey! Not only that but it's only supported on a few platforms!

If you do a grep for BR2_PACKAGE_SPIDERMONKEY in the package directory, only two packages use it. Polkit, and udisks. And udisks only requires spidermonkey because of polkit!

In GitLab by @genexerc on May 17, 2021, 19:32

@Aduskett you stated the problem correctly. Thats why we are gathering here

Can anyone write something against using bash scripts in place of js configs?

In GitLab by @jrybar on May 17, 2021, 20:13

To many in this discussion, I suggest this interesting read (which was a revelation for me too).

Ad updating the test) I'm not specifically against changing the test if the idea of the test stays the same, i.e. catch loops. However keeping the check for exception would be nice (I need to dig in it a bit more).

Anyway thanks everyone for joining this!! I really appreciate it.

In GitLab by @swills on May 17, 2021, 23:04

There's a line in that post that says:

Third, being an interpreted language, we can actually sensibly terminate runaway scripts.

This issue seems to suggest that assertion warrants more detail. Or maybe the test just needs updating? I'm not sure.

Also, I remain nervous about pkexec, since it's suid and links to some larger and more complicated libraries.

In GitLab by @tinywrkb on Jun 3, 2021, 16:37

I've been using this for a while now on my Arch Linux systems, it works correctly as far as I can tell, so it would be great to see this moving forward.
If anyone wants to try it, it's available in the AUR as polkit-duktape, and nothing seems to be broken by my cherry-picking.

In GitLab by @martinetd on Jun 18, 2021, 13:07

Coming in from a request to try making my rootfs smaller on embedded device ( https://gitlab.alpinelinux.org/alpine/aports/-/issues/12771 ), I can only second what's said here even if it's incredibly rude as a drive-by comment...

duktape is much simpler and lightweight so I'm happy seeing this MR - @ferion11's solution is good and @jrybar explicitely gave OK to change the test with this syntax, so it just need a last push... (Unfortunately I won't have time to be the one giving it, sorry - keep it up!)

In GitLab by @Aduskett on Jul 4, 2021, 03:45

Any update on this? Python2 is EOL, which is a huge security issue for Yocto and Buildroot as the spidermonkey version that can cross-compile (without a ton of hacks and tons of extra space) requires python2.

In GitLab by @Aduskett on Jul 20, 2021, 16:48

Considering the lack of response to the above is it safe to surmise that the maintainers do not care about embedded Linux? I made a fork of Polkit in the meantime with the duktape patches applied for Buildroot and Yocto found here: https://github.com/aduskett/polkit-duktape

In GitLab by @jrybar on Jul 20, 2021, 18:06

Hello, creating forks sure is your right in the open community. However, not merging half-finished, unsafe MRs that are WIP and not passing through CI pipeline does not mean it is not desired to replace mozjs, but that polkit should follow some safety and readability rules, as mentioned in my comment.
I hope this MR meets the qualities soon. I'm doing my best to gather time to contribute to the work of the author.
Thanks for the patience of you all.

In GitLab by @Aduskett on Jul 20, 2021, 18:30

Hello Jan; I appreciate the response. As for your comment on "half-finished, unsafe MR" I would disagree. So far in this thread:

Comments and suggestions as to if the tests are even appropriate for duktape (unanswered)
Ignoring comments from multiple people about how the embedded world doesn't like a 20MB .so file just for Polkit.
Comments such as "spidermonkey is already replaced by v8 in modern systems" which again, ignores embedded.
Polkit now requires MozJS > v60 which does not build in Yocto nor Buildroot, so again, the maintainers not thinking/caring about embedded security.
A general lack of engagement with the community.

It's frustrating, to say the least.

Edit*

This quote from a discussion with the Buildroot maintainers puts this MR in perspective:

"Discussions have been sparse, and the polkit maintainer's reaction has not been very supportive, it even feels like they're trying to find every possible argument or small issue to not merge the duktape integration."

In GitLab by @jrybar on Jul 21, 2021, 11:25

on "half-finished, unsafe MR" I would disagree

Like I said in my previous comment, the functionality of runaway_killer MUST be reimplemented, because without it, mal-written .rules files can hang polkitd. End of story.

Comments and suggestions as to if the tests are even appropriate for duktape (unanswered)

I answered this here. I said that I'm not against changing the test if the purpose of the test stays the same, i.e. I'd appreciate a reimplementation of runaway_killer, either with settimer() or in other way, and THEN change the tests so the exception thrown is not checked. I will NOT remove tests just so this MR without runaway_killer passes. That is not the purpose of any test (so it passes every time). I haven't seen any specific code addressing this. You're welcome if you have any.

Ignoring comments from multiple people about how the embedded world doesn't like a 20MB .so file just for Polkit.

Yes, I saw your comment. And you're right, Mozjs is big, Python2 is dead and I'd love to see an alternative too. Yet your comments do not resolve problems with this MR. Other comments suggest switching to another language. If that is a general desire, it should be discussed on higher level than comment section of this specific change.

Comments such as "spidermonkey is already replaced by v8 in modern systems" which again, ignores embedded.

In cooperation with all major distros, we've accomplished many tasks. No one ignores anything. If you want to offer embedded-related contributions, your're definitely welcome.

Polkit now requires MozJS > v60 which does not build in Yocto nor Buildroot, so again, the maintainers not thinking/caring about embedded security.

I'm really sorry that mozjs>60 does not compile on Yocto, but I'm not really responsible for mozjs on that platform or anywhere.

A general lack of engagement with the community.

This really hurts me, not just because I'm trying to address every comment or MR whenever I find some time between other projects I'm part of, but also because this denies the recent release which many people worked on and brings new features and fixes. I realize that the work on new versions may seem slower, however related it is to the work spent on rebasing, or simply testing. It's in my belief that polkit as a security-related project should be a subject to excessive testing and checking to avoid as many vulnerabilities as possible. Hence the following:

This MR hasn't been accepted yet because of two major flaws I have written about many times.
I hoped that those would be addressed by the original author.
Without reimplementing (or reinventing) the functionality of runaway_killer, this MR will not make it into main branch, and this is my final statement.
After a long time working elsewhere I finally found time to tend polkit again. I can see this MR is bigger priority than other ones I've been merging, simply because they're mostly finished and working.
Without any guarantee, some more time can be spent on this one in hope of not having to reply to another accusative and toxic comments.

@Aduskett, I really appreciate that people from the community are part of the polkit project. You're welcome as well, of course, and your contributions are more than appreciated, just as those of all the contributors signed under other merge requests, issues and in CHANGES list.

In GitLab by @karlson2k on Jul 21, 2021, 19:02

@jrybar, thank you for your attention on this MR. It is very desirable to drop so huge dependency from polkit, especially for source-based distributions.

I hope @yetist will find some time to finish this MR. Runaway killer is a must, no doubts.

In GitLab by @ecraven on Aug 31, 2021, 16:07

Is there any way in which someone could pick up this MR and add just the runaway_killer? If @yetist doesn't have the time, maybe someone else could do this? Or is this a problem ethically and/or licensing-wise?

In GitLab by @corvus on Aug 31, 2021, 17:02

it's under GPL, nothing unethical about changing it

In GitLab by @genexerc on Aug 31, 2021, 17:32

Its not that difficult to make runaway killer with ducktape but it obviously would be incompatible with current runaway test implementation. Current implementation wants runaway to break into javascript(generate exception) but that way you can't handle RE stuff properly.

Its easy to make runaway killer which will terminate script execution and exit to polkit, not js. polkit then can safely terminate with error message/code. Is it acceptable?

In GitLab by @jrybar on Aug 31, 2021, 17:51

Absolutely! I'd be satisfied with anything that simply times out when a JS-script loaded from .rules file takes way too long (previous implementation gave it 7 second AFAIK). I was going to write something using something simple from e.g. time.h or whatnot, but I need to take care of !21 first since it's rebased.

In GitLab by @nanonyme on Aug 31, 2021, 18:15

These commits may constitute original work criteria so it may be safest to put same commits into new MR and runaway killer work on top as new commits. @jrybar as maintainer might be able to give more educated opinion on this though if @yetist is not willing to continue getting this merged.

In GitLab by @jrybar on Aug 31, 2021, 22:22

These commits are modifications of a GPL-licenced source file, hence I'd expect, though copyrighted (?, never stated though), the code is a subject to further change in terms of FSF code under inherited GPL licence (which is copyleft), because GPL exist so the source is free to distribute, learn from or change. However, I can provide the context to the legal department.

In GitLab by @jrybar on Sep 3, 2021, 11:18

Thinking of https://gitlab.freedesktop.org/polkit/polkit/-/merge_requests/35#note_1046929, this step is required anyway, as the commits need to be rebased and this usually requires to create new merge request when I do it manually.

In GitLab by @xen0n on Sep 11, 2021, 10:08

I happen to know @yetist personally, and have pinged him. I run into exactly the mozjs dep problem just now; on the new LoongArch architecture LLVM/Rust is unavailable so far, and old mozjs versions require python 2.x to build, and this MR is exactly what everyone bringing up new hardware wants.

In GitLab by @BoQsc on Sep 12, 2021, 14:52

Can we get over the mozjs, spidermonkey already. I really don't care if it comes on popular distributions preinstalled. JavaScript itself is an unfixed hell of a language. Why would you force everyone to have it installed and compiled, especially such a large megabytes wide blob. Make polkit standalone and reasonable as possible or at least do something with this duktape IDK.

In GitLab by @jrybar on Sep 12, 2021, 19:06

Read the thread, you'll get your answers.

In GitLab by @limachaves on Sep 14, 2021, 22:51

mentioned in commit limachaves/polkit@306510777459e2f7fd1452d0a3e8c3bbf6a991ae

vmihalko / t2_polkit

WIP: Add duktape as javascript engine. - [closed] #244